-
-
Notifications
You must be signed in to change notification settings - Fork 358
Auto-merge ruby-builder-bot PRs #848
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
base: master
Are you sure you want to change the base?
Auto-merge ruby-builder-bot PRs #848
Conversation
This PR is trying to drive the conversation from https://bugs.ruby-lang.org/issues/21804. I don't know that it's a good idea to auto-merge on a project so critical in the Ruby supply chain. For a foundational action that runs across thousands of CI pipelines, the blast radius of a bad merge is huge. Auto-merge might be reasonable, but only if it’s tightly scoped to low-risk, mechanically generated changes with strong guardrails. Pros: - Faster propagation of routine updates (e.g., version lists, metadata bumps) without maintainer latency. - Less maintainer toil on high-frequency bot PRs. - More consistent update cadence and fewer stale PRs. Cons: - Single-point-of-failure risk: a compromised bot or supply-chain attack can push a bad change quickly to many downstream users. - Reduced human review on changes that may have subtle security or correctness impacts. - Harder to detect abuse if tests can be manipulated or if the update surface grows over time.
|
Thanks for taking the time to open this PR to move the discussion forward, even though you’re understandably skeptical of the approach. I share your concerns. For context, I’m a lead maintainer for Homebrew, where we make extensive use of automerging across our repositories, but only with significant guardrails. Our merge process is more complex than setup-ruby, but automerge is never fully hands-off. It only occurs after explicit human review and approval. Given the critical role setup-ruby plays in the Ruby supply chain, I think a better path forward is increasing the number of people involved in release management, rather than further reducing human oversight. Adding release managers, as @eregon suggested on the bug tracker, seems like a safer and more sustainable solution. |
| run: | | ||
| gh pr merge "${{ github.event.workflow_run.pull_requests[0].number }}" \ | ||
| --repo "${{ github.repository }}" \ | ||
| --squash \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| --squash \ | |
| --rebase \ |
Because I always use Rebase and merge in this repo
To be clear, it's already the case since @hsbt released 4.0.0. It's up to him whether he is OK with that extra work mentioned in https://bugs.ruby-lang.org/issues/21804#note-2. |
|
This looks good to me. I think the main downside I see is if master is somehow "broken" yet CI passes and some automated PR is made, merged & released then it can break
I'll check if ruby-builder-bot has 2FA and if not add it.
I think it would be good to add a guard that only some files are modified, specifically:
Those are very low risk, i.e. any attacker editing those AFAIK still couldn't do anything bad with it except making workflow fails (e.g. by removing a version). They are all metadata files (.json/.md), or checked generated files, no code. And for Windows:
Those need some validation (or a refactor), because they contain URLs. @flavorjones Maybe you could amend the PR to check only those 5 files are modified?
The check for modified files should address that. I don't expect the update surface to grow, it hasn't in a long time. |
|
I would like to see where the conversation in https://bugs.ruby-lang.org/issues/21804 goes before investing more time in this PR. If it's determined that auto-merging and auto-releasing is an acceptable path, I would be happy to make some of the changes suggested. |
|
Another concern is that the bot PR does not update the test matrix when new major/minor ruby release is available, which can potentially lead to untested broken code getting released out. For example, I just found out that ruby 4.0 on windows is missing from test matrix: #853 Hypothetically, let's say ruby 4.0 on windows build was somehow broken, and the bot PR auto-merged the change because all tests were passing as the broken one wasn't even tested, for end-users who is using This can be addressed either by letting the bot updating the workflow when necessary - this might be tricky given that we use matrix permutations with extra include/exclude. Alternatively, we can use a dynamic matrix - that is to use a job to generate the test matrix from the version files as JSON output, and then use dynamic value For example: prepare-matrix:
runs-on: ubuntu-latest
outputs:
matrix: ${{ steps.matrix.outputs.matrix }}
steps:
- uses: actions/checkout@v6
- id: matrix
run: echo "matrix=$(ruby prepare-matrix-json.rb)" | tee -a "$GITHUB_OUTPUT"
# prepare-matrix-json.rb would generate a JSON matrix one-liner from contents of
# ruby-builder-versions.json and windows-versions.json
test:
needs: [prepare-matrix]
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix: ${{ fromJSON(needs.prepare-matrix.outputs.matrix) }} |
Good catch. I'll note however:
The generated test matrix idea sounds good, especially given that allowing these automated PRs to change the workflow would be rather dangerous. |
|
I would prefer if people commented on the ruby bug tracker (linked in the description) and not here about the general topic of auto-merging. Thanks. |
|
FWIW https://bugs.ruby-lang.org is generally not an appropriate place to report issues related to setup-ruby, but I suppose in this case it's fine enough as it may concern work for other Ruby committers. |
I reacted to this maybe rather strongly, I think the main reasons are:
|
|
@eregon No worries. You're the maintainer and I appreciate your thoughts. I'll push some changes to this PR tonight to implement the additional checks you requested above. |
It seems rather unlikely that CRuby’s release schedule will change. This reads as a suggestion that because no current maintainers want to take this on, the solution is to remove the human element. Would it not make sense to instead add maintainers who are willing to help with this process, rather than removing human oversight altogether? If merging these PRs and cutting a release is considered so critical that it cannot reasonably be supported by expanding the maintainer group, that would seem to argue for keeping human review in place rather than automating it. As an OSS maintainer myself, I am very aware that volunteer maintainers do not owe anything to anyone. When our maintainers are not interested in a particular task, we usually treat that as a signal to invite the broader community to step in and help fill the gap. I believe this argues for more human involvement in critical processes, not less. |
|
As I mentioned in the description, I agree with @p-linnane. I am always working on the native gem / rake-compiler toolchain around the Ruby release date (and the toolchain and the dependent gems can't be fully tested without setup-ruby) so I would be happy to volunteer to fill a release manager role. |
Per conversation in ruby#848, I've added automerge-check.rb which: - checks changed files against a small allowlist - checks the URLs in the windows version and toolchain files The script contains tests for itself which can be run by passing a `--test` flag on the commandline. Note that automerge-check.rb is run from the base branch (master) to avoid malicious PRs from changing the script. Also note that dist/index.js is previously being checked in the "lint" job in test.yml (which this pipeline depends upon).
IMO maintainers of a project should (ideally/when possible) know the project well and e.g. have made several contributions before they become maintainers. However, I'm not sure it would solve this issue, in that they might also not e.g. check their email multiple times a day during the winter holiday break (which IMO is unreasonable to expect of anyone, people deserve holidays/a break). For TruffleRuby, I took care to be able to merge a add-a-release PR for all projects that distribute it (ruby-versions, ruby-build, rvm, setup-ruby), so it's fast and reliable. This is even properly documented. I think it would make sense for CRuby to do the same, i.e. that release managers (@hsbt, @nagachika and @k0kubun), because the release managers are typically available around the time of the release, also take care of these extra steps after the release is published:
@hsbt @nagachika @k0kubun How about it? Regarding Windows, given the timing really depends on @larskanis, I think it would make most sense to ask him (and give him permission) to trigger this workflow, and then wait for the automated setup-ruby PR, CI and merge it and create a release. So if they accept, we could give them write permission. From https://bugs.ruby-lang.org/issues/21804#note-10
Yes, good idea, I think the automated PRs could just ping all these people in their description and also give a link for how to create a release, so it's all self-contained. Hopefully it doesn't amount to too much notifications, but given CRuby, JRuby and TruffleRuby release not very often it should be OK. (BTW, I'm writing here because it's convenient and pings probably work better on GitHub than Redmine) |
|
I consider access to https://github.com/ruby/ruby-builder to be very sensitive, so it'd be great there if we could use a custom role or a PAT or something to only allow triggering certain workflows but not e.g. modify release assets. |
My $0.02: on both of these: I would say my experience (from both sides of this) is it's worth having a lower bar for people who have a demonstrated track record of maintaining open source software operating at a scale of Ruby (or higher). |
This PR is trying to drive the conversation from https://bugs.ruby-lang.org/issues/21804.
I don't think that it's a good idea to auto-merge on a project so critical in the Ruby supply chain. For a foundational action that runs across thousands of CI pipelines, the blast radius of a bad merge is huge.
Auto-merge might be reasonable, but only if it’s tightly scoped to low-risk, mechanically generated changes with strong guardrails (which this PR has tried to do); but if
ruby-builder-botgets compromised then it's game over for the Ruby supply chain.Pros:
Cons: