-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Document use of skip-precommit-approval
label for non-review pull requests
#81053
Conversation
…equests Derived from this discussion: https://discourse.llvm.org/t/prs-without-approvals-muddy-the-waters/76656
llvm/docs/CodeReview.rst
Outdated
If you are using a Pull Request to get precommit CI results, but not | ||
because it needs a precommit review, apply the `skip-precommit-approval | ||
<https://github.com/llvm/llvm-project/labels?q=skip-precommit-approval>` | ||
label to the PR. |
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.
If you are using a Pull Request to get precommit CI results, but not | |
because it needs a precommit review, apply the `skip-precommit-approval | |
<https://github.com/llvm/llvm-project/labels?q=skip-precommit-approval>` | |
label to the PR. | |
If you are using a Pull Request to get precommit CI results or for a revert, but | |
not because it needs a precommit review, apply the `skip-precommit-approval | |
<https://github.com/llvm/llvm-project/labels?q=skip-precommit-approval>` | |
label to the PR. |
(adding revert here)
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.
Oh, yeah - generalized the wording a bit further to mention "purposes other than code review" including code review, convenient web-based reverts, "etc" since there might be other reasons now or in the future that PRs might be used for purposes other than review.
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.
LG!
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.
As far as I'm aware, there hasn't been any work done on implementing this on the subscription side? We should probably wire the label into the subscription job so that we don't end up sending extra notifications (which was part of the original RFC from what I understand), unless I'm missing something and that has already been done.
Honestly, I'd like to have that - though I know next to nothing about how the email subscription job works and at least a label would be better for me than the current state of affairs. So I'd consider this patch to be incremental improvement and further work such as subscription improvements would be quite welcome - I'd even take suggestions/pointers as to where the emails are implemented & I'd have a go at making that improvement. Perhaps @tstellar can point me in the right direction for that? |
Is the goal to not send any notifications if this label is applied to a PR? |
…equests Derived from this discussion: https://discourse.llvm.org/t/prs-without-approvals-muddy-the-waters/76656
Yes - that would be my preference/goal. If I'm not using the PR for review, as much as possible, it should be as if it never had a PR and was just committed - as far as the rest of the community is concerned. |
To do that you would need to add a step here to check for that label and then skip the labler step if it exists. |
Derived from this discussion: https://discourse.llvm.org/t/prs-without-approvals-muddy-the-waters/76656