workflows/check-cherry-picks: post review comments#412068
workflows/check-cherry-picks: post review comments#412068wolfgangwalther merged 6 commits intoNixOS:masterfrom
Conversation
github-actions
bot
added
6.topic: continuous integration
wolfgangwalther
force-pushed
the
ci-cherry-pick-comments
branch
from
8a607e5 to
67ef23f
Compare
github-actions
bot
added
10.rebuild-darwin: 0
wolfgangwalther
force-pushed
the
ci-cherry-pick-comments
branch
from
67ef23f to
672b61d
Compare
nix-owners
bot
requested review from
Mic92,
azuwis,
infinisil,
mweinelt,
risicle and
zowoq
|
I copy&pasted together an example of what the different errors/warnings look like. This was not a real run, but I just didn't feel like re-creating an example that does all 3 failure modes at once. Here's a screenshot:
(the details toggle starts in a closed state, I opened that manually) |
Sometimes we have to modify a commit, for conflict resolution. This is bad, it'll block the PR, as commiters have the permission to "block" the PR by "requesting changes". nixpkgs-ci in the sense is also a commiter. IMO, review "Comment" should be used instead.
|
I disagree. Anyone who can merge a PR can also dismiss a "Request changes" review. This being a manual step encourages the reviewer to actually check whether the review should be dismissed or not. Example
The generated review text explicitly mentions that committers should dismiss the review if it is invalid. |
This comment was marked as duplicate.
This comment was marked as duplicate.
wolfgangwalther
force-pushed
the
ci-cherry-pick-comments
branch
4 times, most recently
from
c58a86d to
9a08b5b
Compare
|
So glad this is getting some more love |
All's well until you have to (force) push multiple times, and you'll be spammed with a message and request changes. This is exactly why 38003ce was reverted . With the reason being:
|
wolfgangwalther
force-pushed
the
ci-cherry-pick-comments
branch
5 times, most recently
from
8b8bbaf to
22d0c0b
Compare
philiptaron
left a comment
philiptaron
left a comment
There was a problem hiding this comment.
Love the pagination!
wolfgangwalther
force-pushed
the
ci-cherry-pick-comments
branch
from
22d0c0b to
176c905
Compare
This comment was marked as resolved.
This comment was marked as resolved.
wolfgangwalther
force-pushed
the
ci-cherry-pick-comments
branch
from
176c905 to
81b5935
Compare
The command substitution style we recently switched to strips trailing newlines, so we don't need to check for empty lines anymore.
This way it's a bit more centralized and easier to extend.
Instead of failing the job, the workflow will now post review comments as "Request Changes". This makes the feedback more readily visible and avoids having to merge despite a failing CI job. It is also a pre-requisite to enable required status checks / required workflows in the future. Committers are asked to confirm the differences by explicitly dismissing the generated review. After dismissal, the related review comment will automatically be marked as "resolved". The comments only report warnings and errors. Reviews are automatically dismissed when they have been addressed by the author and no problems remain. If problems remain, existing, still pending, review comments will be updated. If the same problems had already been dismissed earlier, no new review comment will be created either.
GitHub comments have a length limit, so we can't just dump everything. The 10k limit is arbitrary, but the assumption is that reviewing the range-diff is not the sensible thing to do once it becomes a certain size - reviewing the regular diff and treating the commit as "new" is easier to do in that case. Thus, truncating should work out fine, especially when the full range-diff is still available in the runner log. This could still end up in with an error, if a PR has multiple commits, which all hit the limit. Let's get there first, before we try to fix that hypothetical case, too.
wolfgangwalther
force-pushed
the
ci-cherry-pick-comments
branch
from
81b5935 to
856792f
Compare
|
I think we put enough effort into this to just try it now. We can always iterate later. |
|
Successfully created backport PR for |
github-actions
bot
added
the
8.has: port to stable
6 participants
Instead of failing the job, the workflow will now post review comments as "Request Changes". This makes the feedback more readily visible and avoids having to merge despite a failing CI job. It is also a pre-requisite to enable required status checks / required workflows in the future.
Committers are asked to confirm the differences by explicitly dismissing the generated review. After dismissal, the related review comment will automatically be marked as "resolved".
The comments only report warnings and errors. Reviews are automatically dismissed when they have been addressed by the author and no problems remain. If problems remain, existing, still pending, review comments will be updated. If the same problems had already been dismissed earlier, no new review comment will be created either.
Discussed in #411709 (comment)
Note: The full content of an example comment for this PR can be found in the summary page for the check-cherry-picks workflow. This is an example based on this PR, which is not a backport, so will only raise warnings for each commit, because there is no commit hash to pick. The workflow can't post the comment in this PR, because it runs in the
pull_requestcontext right now, which doesn't have the permissions to do so. Posting review comments will only work once the workflow runs in thepull_request_targetcontext after merge.The example doesn't contain a sample of the "please review the diff" case. In this case there will be a collapsable diff displayed in the warning, which is created as a github codeblock of type
diff. This gives some basic colors for added/removed lines. In the summary page, the commit hashes are display as raw values, but in the comment GitHub will automatically turn them into links to the actual commits, which also allow hovering over them to see the commit message.Things done
Add a 👍 reaction to pull requests you find important.