feat: add support for in tree actions from all configured branches #73
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bhearsum
force-pushed
the
hook-branches
branch
2 times, most recently
from
7da942a to
2139bf6
Compare
|
Update: I did some additionally sanity checking by comparing the json form with and without this change with this script. That showed that the only changes to hooks that already exist are to their descriptions, as expected: With all of this in mind, this should be perfectly safe to take, assuming the approach is desired. |
bhearsum
force-pushed
the
hook-branches
branch
from
2139bf6 to
3d7be02
Compare
ahal
requested changes
Sep 4, 2024
ahal
approved these changes
Sep 6, 2024
bhearsum
force-pushed
the
hook-branches
branch
from
3d7be02 to
8478230
Compare
bhearsum
force-pushed
the
hook-branches
branch
from
8478230 to
188321f
Compare
This secret exists automatically (see https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication#about-the-github_token-secret). We need it set in a to avoid rate limits when fetching lists of branches for repositories in a subsequent commit.
This is a basic implementation that should be fine for now. At some point we'll want to convert this to query as an App so that we can support private repositories, but I don't think it's worth the effort at the moment, as it will require additional plumbing for the app secret, installing to a bunch of repos, etc.
…ories There's a fair amount going on here, but most of it is moving around code (most notably the `branch_name` check in `update_resources` needs to happen before we try to check the level of a branch, as not all branches that exist in Github will be present here). The functional changes here invert our iteration over branches: rather than just assuming all non-globbed ones exist in Github, we fetch the list from Github and check each one to see if it matches one that's configured in projects.yml.
bhearsum
force-pushed
the
hook-branches
branch
from
188321f to
bf5d1d3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I believe this change is overall a good one, but it has some possibly controversial aspects and/or downsides. I'm interested in any and all feedback or pushback. There might be some ways this can be done without some or all of the downsides.
Most notably, this change more or less requires that a
GITHUB_TOKENis set in the environment to runci-adminsuccessfully, otherwise you'll run into rate limits when branches are fetched from Github.The other, less serious, downside is because we generate hooks for all globbed branches, we end up generating many, many superfluous ones for repos like https://github.com/mozilla/application-services, which have many old and now-unused release branches. One way we could improve this is to not use the
brancheslist to determine which branches get action hooks, but have them specified separately. This has its own downside of requiring us to maintain a separate list, and possibly not working automatically when new release branches are created.The
ci-admin difffor this is giant and weird because so many new hooks are being created. One thing I did for additional sanity checking was to diff the list of resource names, which showed a whole bunch of new hooks being added. I'd like to diff hook bodies as well, but https://bugzilla.mozilla.org/show_bug.cgi?id=1905339 is making that difficult.This is built on a few other PRs; only the most recent 3 commits are the relevant changes.