[RFC 0128] Selective auto-merge on bot upgrades #128
Conversation
superherointj
force-pushed
the
rfc-0128-selective-auto-merge-on-bot-upgrades
branch
5 times, most recently
from
39ee7a6
to
244efe1
Compare
There was a problem hiding this comment.
Some points from me:
- it could create an incentive to not update dependencies or improve the package because then you wouldn't get an auto merge
- the review process for updates is already not very great compared to other distros and this wouldn't improve the situation and more likely even worsen it
- I am not sure yet if this could become a footgun for us.
| GitHub Actions will trigger a merge when the following 4 conditions are met: | ||
| 1) `@r-ryantm` is PR's author. | ||
| 2) Package `meta.autoMerge` attribute is enabled. | ||
| 3) All CI checks passed. |
There was a problem hiding this comment.
This would also enforce that the package has right meta.broken/meta.platforms set otherwise darwin failures blocks auto merges.
There was a problem hiding this comment.
Wouldn't having to review & properly set platforms for autoMerge be a good thing? It could be checked/done when enabling the flag.
| 1) `@r-ryantm` is PR's author. | ||
| 2) Package `meta.autoMerge` attribute is enabled. | ||
| 3) All CI checks passed. | ||
| 4) Package maintainer has approved PR. |
There was a problem hiding this comment.
How would that work if a package has multiple maintainers? Would they all need to approve? Would one be enough? Would 50% need to approve? Would we need to prune old maintainers?
There was a problem hiding this comment.
How would that work if a package has multiple maintainers? Would they all need to approve? Would one be enough? Would 50% need to approve?
As it is now, on autoMerge enabled packages, any single maintainer could trigger autoMerge by adding the autoMerge label to PR.
The commiter would have to consider to whom and to which package he is granting auto-merge permission.
The policy can be improved to fullfill needs. Like:
meta.maintainerApprovalsForAutoMerge = 2;
(default = 1)
meta.maintainersGrantedAutoMerge = [ maintainer1 maintainer2 ];
(enabled autoMerge and empty list would grant permission to all maintainers. Empty list being default.)
As an initial proposal, I'd prefer to keep this simple, for a prototype at least. Delaying implementing maintainerApprovalsForAutoMerge and maintainersGrantedAutoMerge. In case of uncertainty on a package or maintainer, or that an unusually complex policy is required, autoMerge shouldn't be enabled (for now at least).
Would we need to prune old maintainers?
Inactive maintainers don't approve PRs. => Not an issue?
There was a problem hiding this comment.
I would default to 50% of the maintainers listed.
| # Drawbacks | ||
| [drawbacks]: #drawbacks | ||
|
|
||
| * Reduced trustworthiness. |
There was a problem hiding this comment.
| * Reduced trustworthiness. | |
| * Upstream maintainers could send software updates with some delay to the master branch then unstable branch/channel and eventually stable branch/channel potentially without *any* interaction or review from a NixOS committer reducing the trustworthiness of updates |
| [drawbacks]: #drawbacks | ||
|
|
||
| * Reduced trustworthiness. | ||
| * Some possible security issue. |
There was a problem hiding this comment.
| * Some possible security issue. | |
| * Possible security issues: | |
| * Changing ofborg from being read-only in the nixpkgs repository (aside from setting CI status) to being a critical security related software where potential bugs in the reviewer request code could lead to rogue merges | |
| * packages with many maintainers and auto merge enabled could receive accidental merges from older maintainers not fully aware of the auto merge feature yet |
There was a problem hiding this comment.
- Changing ofborg from being read-only in the nixpkgs repository (aside from setting CI status) to being a critical security related software where potential bugs in the reviewer request code could lead to rogue merges
OfBorg was restricted to adding labels and is absent of merge process.
OfBorg is no longer labelling auto-merge, the maintainer is now responsible to add the auto-merge label.
The auto-merge label only trigger GitHub Actions to run, rules enforcement and merge is done by some code yet to be defined in GitHub Actions. Any failure in the code used to enforce rules and merge would be a security problem, so I've added a mention for it. (Security issues: Possible failures in the GitHub Action code used to enforce rules and merge.)
- packages with many maintainers and auto merge enabled could receive accidental merges from older maintainers not fully aware of the auto merge feature yet
Removed OfBorg reference. And added:
Maintainer adds "auto-merge" label to PR triggering GitHub Actions to enforce rules and merge.
| # Alternatives | ||
| [alternatives]: #alternatives | ||
|
|
||
| * https://github.com/NixOS/rfcs/pull/50/ |
There was a problem hiding this comment.
| * https://github.com/NixOS/rfcs/pull/50/ | |
| * The Marvin MK II experiment ended and didn't yield a noticeable improvement. |
There was a problem hiding this comment.
The closest alternative I found was #50, should I really remove it?
- The Marvin MK II experiment ended and didn't yield a noticeable improvement.
How does this sentence relate to this RFC?
There was a problem hiding this comment.
How does this sentence relate to this RFC?
it had a similar goal to improve the review and merge situation
superherointj
force-pushed
the
rfc-0128-selective-auto-merge-on-bot-upgrades
branch
4 times, most recently
from
cfe3e2b
to
fb85f0c
Compare
superherointj
force-pushed
the
rfc-0128-selective-auto-merge-on-bot-upgrades
branch
from
fb85f0c
to
5b8f79f
Compare
| # Detailed design | ||
| [design]: #detailed-design | ||
|
|
||
| Add a new `meta.autoMerge` package attribute with type `bool` defaulting to `false`. To be documented in Nixpkgs manual. |
There was a problem hiding this comment.
If such a attribute/flag is to be implemented, I don't think it should be a meta ket, but a passthru one.
meta, as the name suggests, it for metadata about the package, not about how our particular infrastructure would treat such package.
Even if some meta flags are useful to our infra - license defines if something can be cached or not, but has an informational nature independent of it; the same can be said about broken and badPlatforms.
Maybe meta.hydraXXX is a notable exception.
|
If you accept some form of suggestion, I think that some "keyword" can be used for this. If a committer and/or a maintainer of the package believes it is OK to merge, he can throw a message like
and the merge bot will merge it at the specified date, with The merge bot will include the name of the one that issued the OK on the commit message. This way we can trace the responsibilities. |
| Maintainer adds `auto-merge` label to PR which triggers GitHub Actions to enforce rules and merge. | ||
|
|
||
| Rules are: | ||
| 1) `@r-ryantm` is PR's author. |
There was a problem hiding this comment.
Is it currently possible for other committers to push to a @r-ryantm branch? If so, does it get possible then to hijack a PR and (force) push something different? (Of course committers can do that already)
There was a problem hiding this comment.
Yes, it is possible and I have do this on many occasions to fixup things.
|
I'd prefer to close this for now. And resume in case it still makes sense later. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Rendered