Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci(system-file-changes): pass if author/actor is an admin #38610

Merged
merged 7 commits into from
Mar 18, 2025

Conversation

caugner
Copy link
Contributor

@caugner caugner commented Mar 13, 2025

Description

Prevents the system-file-changes workflow from failing, if both PR author and actor are admin.

Motivation

Reduces noise when admins update system files, and reduces the chances of missing other failing workflows.

Additional details

Verified as follows:

  • Temporarily added pull_request trigger, see: https://github.com/mdn/content/actions/runs/13838682590/job/38720221491
  • Manually ran:
    AUTHOR="mdn-bot"
    AUTHOR_PERMISSION=$(gh api https://api.github.com/repos/mdn/content/collaborators/$AUTHOR/permission --jq .permission)
    
    if [ "$AUTHOR_PERMISSION" != "admin" ]; then
     echo "PR author ($AUTHOR) is not an admin, please ping someone for a review."
    fi
    # Output: PR author (mdn-bot) is not an admin, please ping someone for a review.

Related issues and pull requests

@caugner caugner requested a review from a team as a code owner March 13, 2025 15:32
@github-actions github-actions bot added system [PR only] Infrastructure and configuration for the project size/s [PR only] 6-50 LoC changed labels Mar 13, 2025
@Josh-Cena
Copy link
Member

Josh-Cena commented Mar 13, 2025

I don't see why this workflow is necessary in the first place. Currently infrastructure updates are mostly reviewed by either Brian or me and I'm not an admin. If you don't mark this workflow as required (like what we already do) then it only has the same security requirements as normal content PRs, just that I have to merge it with a red cross. If you do make it required, then it's just making those PRs move slower with an unclear benefit (status quo is "no protection" and you don't see anything going wrong). And currently this workflow blocks on non-"system" files as well, like vscode config, spell checker infra, etc., which is not going to do us a favor.

@caugner
Copy link
Contributor Author

caugner commented Mar 13, 2025

I don't see why this workflow is necessary in the first place.

The value of this workflow is that reveals very prominently if a PR changes system files, and drastically reduces the chances of accidental or intentional system file changes slipping through as part of a "regular" content PR.

In my opinion there are certain files that shouldn't be changed without admin involvement, so it's probably a good idea to make this workflow required.

However, there may be files that are currently classified as system files that don't require this scrutiny.

@caugner
Copy link
Contributor Author

caugner commented Mar 14, 2025

@bsmth found this issue which provides some more context.

Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems solid to me, at least fewer red builds / checks are welcome

@caugner caugner merged commit eaeb5c2 into main Mar 18, 2025
11 of 12 checks passed
@caugner caugner deleted the allow-system-file-changes-by-admins branch March 18, 2025 13:31
bramus pushed a commit to bramus/mdn-content that referenced this pull request Apr 1, 2025
Prevents the system-file-changes workflow from failing, 
if both PR author and actor are admin.

Reduces noise when admins update system files, and 
reduces the chances of missing other failing workflows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/s [PR only] 6-50 LoC changed system [PR only] Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants