WIP: Automated android branch builds #33
Conversation
complexspaces
force-pushed
the
automated-android-branch
branch
16 times, most recently
from
2237dd9
to
ba8de84
Compare
|
This will need some cleanup to checkout the correct branch With that said, this confirms the general idea works. You can see a commit with the pre-built maven repository here: a54d920 |
complexspaces
force-pushed
the
automated-android-branch
branch
2 times, most recently
from
7c9586f
to
5bf2f42
Compare
complexspaces
force-pushed
the
automated-android-branch
branch
from
5bf2f42
to
036ac7e
Compare
There was a problem hiding this comment.
Cool stuff, thank you :-)
I'm a little bit nervous about GitHub workflows that use the $GITHUB_TOKEN to write back to the repository, but I think the risk/reward tradeoff is sensible here and we can approach changes with care.
If there's a way to use branch protection rules (or some other mechanism) to further limit the workflow to only being able to operate with the Android maven branches we intend that would be a nice extra addition 🤞
| # types: | ||
| # - closed |
There was a problem hiding this comment.
Tagging this as a place to fixup pre-merge. I see this types arg un-commented in the upstream docs.
|
|
||
| jobs: | ||
| update-android-branch: | ||
| # if: github.event.pull_request.merged == true |
|
|
||
| git add --force android-release-support/* | ||
| git commit -am "[Automated] Bundle Android component artifacts" | ||
| # git push -f origin main-with-maven |
| #pull_request: | ||
| # merge_group: | ||
| #schedule: | ||
| # - cron: '0 18 * * *' |
There was a problem hiding this comment.
Is there a reason to disable these? Leftover from testing?
There was a problem hiding this comment.
This is just a testing artifact, yes. I didn't want to waste CI resources when I was pushing to this branch a lot during development. I'll make sure to undo this before merging.
| permissions: | ||
| contents: write |
There was a problem hiding this comment.
Maybe prudent to leave a warning at the top of this YML that changes to this workflow should be carefully vetted since it can write to the repository.
I wonder if branch protection rules to require pull-requests into main would protect against this workflow pushing main or if that would be possible through malice or bugs in the script :-/
There was a problem hiding this comment.
I believe branch protection rules do apply to Workflow Actions as well, although I'm unsure we can exactly test the existing main protection rule without risking a little bit.
Following this comment from you:
If there's a way to use branch protection rules (or some other mechanism) to further limit the workflow to only being able to operate with the Android maven branches we intend that would be a nice extra addition 🤞
I tried adding a custom ruleset (see here) to the repository that applies to every branch except main-with-maven. I then tried to push from the CI job to a testing branch. It failed to push with a permissions error like we'd want.
The downside of it seems to be that it treats our pushes to working branches in the repository as rule bypasses as well, which causes extra noise locally 😞
remote: Bypassed rule violations for refs/heads/automated-android-branch:
remote:
remote: - Cannot update this protected ref.
remote:
To github.com:rustls/rustls-platform-verifier.git
036ac7e..e147cb2 automated-android-branch -> automated-android-branch
There was a problem hiding this comment.
The downside of it seems to be that it treats our pushes to working branches in the repository as rule bypasses as well, which causes extra noise locally
Interesting! Thanks for testing.
I'm in the habit of pushing branches to a fork and opening PRs from there, so I don't think the noise would bother me but I can see how its annoying for others that don't want the overhead of managing branches on a fork.
There was a problem hiding this comment.
To clarify, I am personally fine with the noise since I know what it means, and the gained benefit of locking this job down tightly feels very worth it. If I ever get tired of it I could also switch to using a fork. IMO given that we should keep the rule before merging this and live with the very minor downsides.
There was a problem hiding this comment.
We could also consider adding a CODEOWNERS rule to this file to require another approval to prevent accidentally (though not malicious) updates to this workflow that are incorrect and have security consequences.
There was a problem hiding this comment.
We could also consider adding a CODEOWNERS rule to this file to require another approval
I wouldn't mind that extra caution if you wouldn't :-)
This PR is a followup to #32, in order to reduce friction around Cargo Git dependencies.
To create this, these changes introduce a new GitHub Action workflow that runs on every successful merge to main. The action follows these high-level steps:
main-with-mavenbranch state to matchmain.git add --forceto include the generated files and artifacts.main-with-mavenbranch with the new artifacts.As the end result, there is a branch with identical contents to
crates.ioreleases. As noted in the parent PR, this allows us to keep some of the nice properties of Cargo's Git dependency management and avoid friction between true releases.One open question is if this should wipe out
main-with-maven's Git history each time or if it should perform agit mergeinstead. The upsides of the former are that its simple, clean over time, and easy to follow. The downside is that a merge tomainmay render that commit revision useless for those. It should be available for quite some time though unless GitHub intervenes and manually purges the repository cache. I lean towards wiping it every time given the above.