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.

Sign up for GitHub

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

Jump to bottom

[RFC 0128] Selective auto-merge on bot upgrades #128

Closed
wants to merge 1 commit into from
Closed

[RFC 0128] Selective auto-merge on bot upgrades #128

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions rfcs/0128-selective-auto-merge-on-bot-upgrades.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
---
feature: Selective auto-merge on bot upgrades
start-date: 2022-07-07
author: superherointj
co-authors: (find a buddy later to help out with the RFC)
shepherd-team: (names, to be nominated and accepted by RFC steering committee)
shepherd-leader: (name to be appointed by RFC steering committee)
related-issues: (will contain links to implementation PRs)
---

# Summary
[summary]: #summary

Introduce a `meta.autoMerge` attribute to packages to allow committers to delegate merge rights to package maintainers for bot auto updates.

# Motivation
[motivation]: #motivation

* Reduce pending PRs for review and merge in nixpkgs.
* Save reviewers and commiters time.
* Speed up package upgrades.
* Motivate reviewers into becoming maintainers and reviewing PRs.

# Detailed design
[design]: #detailed-design

Add a new `meta.autoMerge` package attribute with type `bool` defaulting to `false`. To be documented in Nixpkgs manual.
Copy link
Member

Choose a reason for hiding this comment

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

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.


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.
Copy link
Member

@FRidh FRidh Jul 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is possible and I have do this on many occasions to fixup things.

2) Package `meta.autoMerge` attribute is enabled.
3) All CI checks passed.
Copy link
Member

Choose a reason for hiding this comment

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

This would also enforce that the package has right meta.broken/meta.platforms set otherwise darwin failures blocks auto merges.

Copy link
Author

@superherointj superherointj Jul 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Wouldn't having to review & properly set platforms for autoMerge be a good thing? It could be checked/done when enabling the flag.

4) Package maintainer has approved PR.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@superherointj superherointj Jul 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I would default to 50% of the maintainers listed.


Due maintainers being able to add labels in GitHub, labels are untrusted and are only used to trigger GitHub Actions.
An extra check of the 4 conditions (without using labels) is necessary in GitHub Actions before merge can happen.

The appropriateness of setting `meta.autoMerge` is left up to committers. Commiters have to consider to whom and to which package is being granted auto-merge permission.

# Drawbacks
[drawbacks]: #drawbacks

* Reduced trustworthiness.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* 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

* Security issues:
- Possible failures in the GitHub Action code used to enforce rules and merge.

# Alternatives
[alternatives]: #alternatives

* https://github.com/NixOS/rfcs/pull/50/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* https://github.com/NixOS/rfcs/pull/50/
* The Marvin MK II experiment ended and didn't yield a noticeable improvement.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

How does this sentence relate to this RFC?

it had a similar goal to improve the review and merge situation


# Unresolved questions
[unresolved]: #unresolved-questions

* Reduced trustworthiness.
* Security risks.
superherointj marked this conversation as resolved.
Show resolved Hide resolved