-
-
Notifications
You must be signed in to change notification settings - Fork 11
Switch to PR-based process #34
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
Conversation
d39094d to
4c40b6c
Compare
wolfgangwalther
left a comment
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.
Thanks for doing this. I didn't check the implementation in detail, but approve on the change to a PR-based workflow!
MattSturgeon
left a comment
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.
I tried to read through the diff in a bit more detail. I can't see any issues beyond the minor discussion points below, most of which are ideas for future improvements or observations of potential (minor) pain points that may not be easily solved.
(I also tried testing on your test org, but ran into the issues we discussed before, as your test issue is still locked.)
It may be worth addressing the regex, validating that the created file is empty and named for a "real" user, and/or tweaking the nomination instructions. But only if that is low effort and doesn't increase this PR's scope.
LGTM, approving. Thanks for working on this, I'm convinced it'll be a much better experience for all involved!
jtojnar
left a comment
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.
Thanks. This looks good.
Regarding #23 (comment):
A PR can easily be approved by individual commit delegators (if they wish to use such a process), which gives more transparency and accountability. It also means that the team doesn't need its own internal synchronisation.
We will still probably want to keep an internal log with notes but it is cleaner to have single PR per nomination rather than issue + PR.
Slight downside is that GitHub requires having a fork in order to send PR but I guess that is acceptable price to pay.
|
PR is now updated with all suggestions addressed :)
I unlocked the issue, so that should work now if you wanna try again |
MattSturgeon
left a comment
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.
PR is now updated with all suggestions addressed :)
LGTM 🚀
I also tried testing on your test org, but ran into the issues we discussed before, as your test issue is still locked.
I unlocked the issue, so that should work now if you wanna try again
Tested here: infinisil-test-org/nixpkgs-committers#49 (CI run)
It does it automatically when rendered
|
Resolved conflict after NixOS/nixpkgs#45 |
|
Do I understand correctly that this is only pending approval by @winterqt? Thanks for the preparation, looking forward to the improved process! |
|
Bump! It would be a shame for this to stall now, so close to the finish line. |
winterqt
left a comment
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.
Thanks for all the work on this! Sorry for the delay.
|
(I will merge + implement this tomorrow ( |
|
Thanks! I'll go ahead with merging and implementing this as described in the PR description :) |
Migrating self-nomination from #50 (comment) as part of switching to PR-based process: #34 Co-Authored-By: Gutyina Gergő <gutyina.gergo.2@gmail.com>
Migrating self-nomination from #50 (comment) as part of switching to PR-based process: #34 Co-Authored-By: Infinidoge <infinidoge@inx.moe>
Migrating nomination from #50 (comment) as part of switching to PR-based process: #34 Co-Authored-By: LordGrimmauld <grimmauld@grimmauld.de>
Migrating self-nomination from #50 (comment) as part of switching to PR-based process: #34 Co-Authored-By: Jonathan Davies <jpds@protonmail.com>
Migrating nomination from #50 (comment) as part of switching to PR-based process: #34 Co-Authored-By: Guanran Wang <guanran928@outlook.com>
Migrating nomination from #50 (comment) as part of switching to PR-based process: #34 Co-Authored-By: Wolfgang Walther <walther@technowledgy.de>
Migrating nomination from #50 (comment) as part of switching to PR-based process: #34 Co-Authored-By: superherointj <sergiomarcelo@ya.ru>
Migrating nomination from #50 (comment) as part of switching to PR-based process: #34 Co-Authored-By: Martin Weinelt <hexa@darmstadt.ccc.de>
Migrating self-nomination from #50 (comment) as part of switching to PR-based process: #34 Co-Authored-By: Heitor Augusto <44377258+HeitorAugustoLN@users.noreply.github.com>
|
Now done effectively everything done as described in the PR description:
|
|
Yes! YES! Amazing to see this land. |
|
I like the idea of using review approvals instead of endorsement comments, as @philiptaron has just done on one nomination. Is there a preference on which to use? |
|
My view is that we only used |
|
I have migrated two more relevant issues from the org repo to here: I left NixOS/org#121 in the org repo, because how the commit bit team works is described over there. I am unsure about NixOS/org#122. It doesn't really belong in the org repo, it would be entirely lost in nixpkgs itself - maybe we should move it here as well? |
This comment has been minimized.
This comment has been minimized.
|
Another reason that this is good: it should make it easier for mentors to find out when they've been assigned to someone. I did not know that I was assigned to someone until several months after the fact (I subscribed myself to the Committers issue but didn't usually monitor every single notification in that thread). (Side note: I did inadvertently end up "mentoring" her, but a dedicated notification on a PR would've given me an opportunity to be more proactive with that mentoring). |
Changes the nomination process to individual PRs and as such requires approval from the entire @NixOS/commit-bit-delegation team. This is effectively an alternative to NixOS/nixpkgs#23.
New process
Feel free to try it out (no worries, this is using a test org, and nobody gets pinged):
When such a PR is opened, an automatic comment is posted in NixOS/nixpkgs#35, allowing people to easily subscribe to new nominations without other noise.
To do after merging
Notes
There is no PR template for now. While this could be done (though there's only a single default PR template per repo), this PR is kept as minimal as possible, focusing only on the process switch from a single issue to multiple PRs.
Furthermore, we could also add instructions for the contributions script or even run it directly. I'd like to defer that to a later PR though, as it's not essential to have for the PR workflow.