ci/request-reviews: request reviewers 1-by-1 #370857
Conversation
Now that we have maintainer reviews as well, be a bit more explicit about naming.
This is now re-used for both code owners and maintainers.
Odd to have this in the "Tagging pull request" step, which is only about labels otherwise.
github-actions
bot
added
6.topic: policy discussion
6.topic: continuous integration
nix-owners
bot
requested review from
azuwis,
LeSuisse,
Mic92,
mweinelt,
philiptaron,
risicle and
zowoq
github-actions
bot
added
10.rebuild-darwin: 0
|
|
||
| log "Requesting reviews from: $(<"$tmp/reviewers.json")" | ||
|
|
||
| if ! response=$(effect gh api \ |
There was a problem hiding this comment.
The one thing this doesn't preserve yet is the effect part here, which implements dry mode, which is used to avoid requesting reviews from code owners for draft PRs.
There was a problem hiding this comment.
Added two fixups to put back the effect part, let me know when they are OK to squash.
This brings up the question: Should we enable DRY_MODE for maintainer requests, too?
There was a problem hiding this comment.
The fixups look good to me!
Should we enable DRY_MODE for maintainer requests, too?
Yeah definitely. Will involve mirroring
andnixpkgs/.github/workflows/codeowners-v2.yml
Lines 34 to 35 in a1945f7
There was a problem hiding this comment.
Just squashed the commits for now, no other changes. Will add the DRY_MODE in a separate PR.
wolfgangwalther
force-pushed
the
ci-request-reviews-1-by-1
branch
from
2a66936 to
1526a4b
Compare
This makes it easier to add ofborg's request-1-by-1 logic, where failed requests are OK for edge cases.
This is to be able to ignore the odd failure for some users, who are listed as collaborators, but still fail to be requested properly.
wolfgangwalther
force-pushed
the
ci-request-reviews-1-by-1
branch
from
1526a4b to
034613f
Compare
|
Testing in Infinisil-s-Test-Organization#41 and Infinisil-s-Test-Organization#42 |
|
Backport added in #370709. |
|
No problem it happen :-) |
I think that only happened on the change of the target branch. Since the new PR is already targeting staging, you should be good to go. I hope. |
|
OK testing now. |
Not related to code-owners, it pinged all the maintainers when changing the target branch without proper rebasing. So the result is kind of the same as with the old code-owners. This didn't come up before, because the request would just fail with more than 15 reviewers. The 1-by-1 approach in this PR works around that limit. A quick fix is to add the same limit that ofborg had to limit the number of reviewers added at the same time. I am working on it. |
The rebase was done correctly, I even took care to move the PR in draft just before doing it. |
Yeah, I didn't mean to imply you did something wrong, sorry. The maintainer review requests don't consider draft mode, yet - so that didn't do anything. |
Requesting reviewers 1-by-1 works around odd failures in edge cases when some reviewers can't be requested, even though they are collaborators.
Should fix / work around #370456 (comment).
First few commits are only some refactors, last commit is the actual change.
Don't know how to test this, seems like I would need to set up more stuff, including a GitHub App etc. in my own fork? Maybe that's easier for you to test, @infinisil?
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.