Fix sync labels issues for step 2 going live completion #36292
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
|
Will this fix the problem with the bot setting a "needs review" when the PR has been approved with a "positive review" set? See #36128. Frankly, I see this as the bot is very broken, and it is quite frustrating that the bot is really that dumb. |
I'm very sorry about the trouble. It was not planned to activate the reaction on removing a label, yet. It happened accidentially (see #36213 (comment)) and I don't have the permission to disable it again. Thus we have to wait until @tobiasdiez or some other maintainer does it. This PR is about a bug concerning setting a label. It should have been fixed already in PR #36213 but unfortunately two lines where missing. Maybe I should remove the |
|
Somewhere in your script, you wrote "state-label". I think "state label" (without hyphen) reads better, like "priority label". |
|
Sorry, I indeed accidentally introduced a typo which lead to the |
I suspected that you mean that, but I can't find it: gives no results. |
|
It is generated here: def reject_label_removal(self, item):
r"""
Post a comment that the given label can not be removed and select
a corresponding other one.
"""
if type(item) == State:
sel_list = 'state'
else:
sel_list = 'priority'
--> self.add_warning('Label *%s* can not be removed. Please add the %s-label which should replace it' % (item.value, sel_list))
self.add_label(item.value)
return |
5 tasks
* fix_on_label_removal initial * fix_on_label_removal state -> status * fix bug in actor valid * once again * rewrite authors in actor_valid * syntax * replace warning by hint
|
I've merged some more changes that I tested in my fork repository in PR soehms#10 (see soehms#10 (comment) for the test results). In short: the |
kwankyu
approved these changes
Sep 25, 2023
|
Btw it should be possible to have the bot accept/reject PRs without any comment (at least through the cli, not sure about the python api). For an example, |
Thanks! Unfortunately we don't have an alternative chance to test, since the behavior in a fork repository is different. |
Yes, that works: The restriction only applies to request changes. Shall I remove the body of the approval? Maybe in the next PR? |
|
Please do it here. |
|
For "request changes" triggered by "s: needs work" label, could the comment be "owned" by the one who added the label? Then the comment could be edited to contain the reason of the the "needs work". |
This was referenced Sep 26, 2023
Closed
I've opened soehms#11 for this. You test it there if you just add |
In my fork I can edit such review comments of the GitHub bot: But probably this works since I'm the owner. For the The information you can obtain from the REST-API for the review shown in the screenshot is this: From the docs I don't see any function that would allow such a manipulation. |
I don't have enough permissions to do that in your repo. If it works for you, I am okay. |
OK. |
I've added you as collaborator. Can you try again?
I can't do it since I'm the author (see soehms#11 (comment)). |
|
I tried again, as collaborator. There are still problems. |
* approve_without_body initial * add missing positional arg * nochmal * issue 2 * Introduce SYNC_LABELS_BOT_TOKEN and fix issue 1 * Fix in gh_cmd because of mark_as_ready * Ignore trigger of bot * Ignore actor's reviews on needs review * dismiss_bot_reviews and refrain from removing labels * fix broken call of review_by_actor * add get_review_requests * consistent use of state versus status * add full stops in comments
|
Is this ready? |
|
Documentation preview for this PR (built with commit b330790; changes) is ready! 🎉 |
From my point of view: Yes! I've merged soehms#11 into this PR. Thus, everything we discussed there is done. Sorry, that I forgot to say this explicitly. |
|
OK. Then let's get in. |
Thanks! |
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 28, 2023
sagemathgh-36292: Fix sync labels issues for step 2 going live completion <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> Unfortunately the fix in sagemath#36213 has not been completed properly. There is still a `return` from the `get_review_decision` method which is not `None` where it should be. This is fixed here. Sorry for this unnecessary trouble and my lack of attention! In a next step this PR fixes a bug which has been noticed by an accidentally activation of the `unlabeled` trigger (see sagemath#36292 (comment)). Simultanously the `labeled`-trigger has been disabled. This is an unrealistic setting which was not covered by the code. Explicitely, a state label could not be removed even though other state labels where set (examples sagemath#36128 and sagemath#36020). This PR leads to the following changes of behavior: 1. The reaction on the `unlabeled`-event is reduced to the case of the last label 2. The `unlabeled`-event will not lead to a rejection of the removal any more 3. Instead of a warning comment a hint comment is posted. We found a way to test the bot a little more realistically (see soehms#11). Additional problems emerged and were also resolved. This led to the following changes in the workflow: 4. A bug in `actor_valid` is fixed (see soehms#10 (comment)) 5. Preparing to use our own bot user (e.g. [sagemathadmins](https://github.com/sagemathadmins)) (see soehms#11 (comment)) 6. Waiver of observing the `reviewDecision` feature provided by GitHub (see 2. in soehms#11 (comment)) 7. Allow the user to revert his decision of label selection (see soehms#11 (comment)) 8. Don't reject label addition any more, except in the case where the author tries to set `s: positive review` to his own PR which has no reviews from others (see soehms#11 (comment)) 9. Dismiss stale reviews of the bot after a push to the branch and on submission of a new review which is more than just a comment (see soehms#11 (comment)). Despite the fact that the testing was now more realistic, it is still not guaranteed that the bot's behavior in `sagemath/sage` will be completely covered by our testing. The changes in this PR cannot be activated immediately after merging into the develop branch due to a bug in the GitHub web interface observed during testing in soehms#11 (see soehms#11 (comment) and soehms#11 (comment)). The problem is that the panel at the top right of the webpage that contains the labels does not update immediately after the bot changes the labels. Since this might cause confusion, I'll create a bug report about it. The going-live of the sync bot will be stalled at least until we received an answer. This PR changes the following flowcharts from sagemath#35172 : ##### What happens when adding `s: needs review` to a PR? ```mermaid --- title: add the needs review label --- flowchart LR %% vertices trigger([label\n 's: needs review'\n added]) mark_as_ready([mark as\n ready for\n review]) remove_other_labels([remove other\n state labels]) warning_about_label_addition([warning\n about label\n addition]) needs_review_valid[[needs review\n valid?]] is_draft{is PR\n a draft?} %% edges trigger --> needs_review_valid needs_review_valid -- true ---> remove_other_labels needs_review_valid -- false ---> is_draft is_draft -- yes ---> mark_as_ready is_draft -- no ---> warning_about_label_addition mark_as_ready --> remove_other_labels ``` The warning is posted as a comment which will be deleted after 5 minutes. ```mermaid --- title: needs review valid? --- flowchart LR %% vertices needs_review_valid([needs review\n valid?]) true([true]) false([false]) latest_review_by_actor{latest review\n by actor} needs_work_valid[[needs work\n valid?]] positive_review_valid[[positive review\n valid?]] is_draft{is PR\n a draft?} %% edges needs_review_valid ---> is_draft is_draft -- yes ---> false is_draft -- no ---> latest_review_by_actor latest_review_by_actor -- yes ---> true latest_review_by_actor -- no ---> needs_work_valid needs_work_valid -- true ---> false needs_work_valid -- false ---> positive_review_valid positive_review_valid -- true ---> false positive_review_valid -- false ---> true ``` ```mermaid --- title: needs work valid? --- flowchart LR %% vertices needs_work_valid([needs work\n valid?]) true([true]) false([false]) latest_review_request_changes{latest proper\n review requests\n changes?} %% edges needs_work_valid --> latest_review_request_changes latest_review_request_changes -- yes ---> true latest_review_request_changes -- no ---> false ``` ```mermaid --- title: positive review valid? --- flowchart LR %% vertices positive_review_valid([positive review\n valid?]) true([true]) false([false]) latest_proper_review_approved{latest proper\n review\n approved?} %% edges positive_review_valid --> latest_proper_review_approved latest_proper_review_approved -- yes ---> true latest_proper_review_approved -- no ---> false ``` Here, proper means that the review is more than a comment. ##### What happens when adding s: needs work to a PR? ```mermaid --- title: add the needs work label --- flowchart LR %% vertices trigger([label\n 's: needs work'\n added]) request_changes([request changes]) remove_other_labels([remove other\n state labels]) warning_about_label_addition([warning\n about label\n addition]) needs_work_valid[[needs work\n valid?]] is_draft{is PR\n a draft?} %% edges trigger --> needs_work_valid needs_work_valid -- true ---> remove_other_labels needs_work_valid -- false ---> is_draft is_draft -- yes ---> warning_about_label_addition is_draft -- no ---> request_changes request_changes --> remove_other_labels ``` ##### What happens when adding `s: positive review` to a PR? ```mermaid --- title: add the positive review label --- flowchart LR %% vertices trigger([label\n 's: positive review'\n added]) positive_review_valid[[positive review\n valid?]] approve_pr([approve]) remove_other_labels([remove other\n state labels]) actor_valid[[actor valid?]] approve_allowed[[approve\n allowed?]] warning_about_label_addition([warning\n about label\n addition]) reject_label_addition[[reject\n label\n addition]] %% edges trigger --> positive_review_valid positive_review_valid -- yes ---> remove_other_labels positive_review_valid -- no ---> actor_valid actor_valid -- yes ---> approve_allowed actor_valid -- no ---> reject_label_addition approve_allowed -- yes ---> approve_pr approve_allowed -- no ---> warning_about_label_addition approve_pr --> remove_other_labels ``` The boxes `actor_valid` and `reject_label_addition` are unchanged. ```mermaid --- title: approve allowed? --- flowchart LR %% vertices trigger([approve\n allowed?]) true([true]) false([false]) review_of_others_request_changes{changes\n requested by\n someone\n else exists?} %% edges trigger --> review_of_others_request_changes review_of_others_request_changes -- yes ---> false review_of_others_request_changes -- no ---> true ``` Here, only reviews of someone else are considered which are more recent than any commit. ##### What happens when a state label is added to an issue? ```mermaid --- title: add a state label to an issue --- flowchart LR %% vertices trigger([label added]) warning_about_label_addition([warning\n about label\n addition]) nothing([do nothing]) is_needs_info{is label\n 's: needs info'?} %% edges trigger --> is_needs_info is_needs_info -- yes ---> nothing is_needs_info -- no ---> warning_about_label_addition ``` ##### What happens when removing a state label from a PR? ```mermaid --- title: remove a state label --- flowchart LR %% vertices trigger([label removed]) nothing([do nothing]) hint_about_label_removal([hint\n about label\n removal]) other_state_label{other state\n labels exist?} is_needs_info{is label\n 's: needs info'?} %% edges trigger --> other_state_label other_state_label -- yes ---> nothing other_state_label -- no ---> is_needs_info is_needs_info -- yes ---> nothing is_needs_info -- no ---> hint_about_label_removal ``` The hint is postet as a comment which will be deleted after 5 minutes. ##### What happens when adding a priority label? ```mermaid --- title: add a priority label --- flowchart LR %% vertices trigger([label added]) remove_other_labels([remove other\n priority labels]) %% edges trigger --> remove_other_labels ``` ##### What happens when removing a priority label? ```mermaid --- title: remove a priority label --- flowchart LR %% vertices trigger([label removed]) nothing([do nothing]) hint_about_label_removal([hint\n about label\n removal]) other_prio_label{other priority\n labels exist?} %% edges trigger --> other_prio_label other_prio_label -- yes ---> nothing other_prio_label -- no ---> hint_about_label_removal ``` ##### What happens when a PR is approved? ```mermaid --- title: approve --- flowchart LR %% vertices trigger([approve\n]) select_positive_review(['s: positive review'\n label selected]) nothing([do nothing]) positive_review_valid[[positive review\n valid?]] pending_review_requests{pending\n review requests\n exists?} actor_authorized{is actor\n a member of\n Triage?} %% edges trigger ---> pending_review_requests pending_review_requests -- yes ---> nothing pending_review_requests -- no ---> actor_authorized actor_authorized -- yes ---> positive_review_valid actor_authorized -- no ---> nothing positive_review_valid -- true ---> select_positive_review positive_review_valid -- false ---> nothing ``` ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36292 Reported by: Sebastian Oehms Reviewer(s): Kwankyu Lee
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 28, 2023
sagemathgh-36292: Fix sync labels issues for step 2 going live completion <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> Unfortunately the fix in sagemath#36213 has not been completed properly. There is still a `return` from the `get_review_decision` method which is not `None` where it should be. This is fixed here. Sorry for this unnecessary trouble and my lack of attention! In a next step this PR fixes a bug which has been noticed by an accidentally activation of the `unlabeled` trigger (see sagemath#36292 (comment)). Simultanously the `labeled`-trigger has been disabled. This is an unrealistic setting which was not covered by the code. Explicitely, a state label could not be removed even though other state labels where set (examples sagemath#36128 and sagemath#36020). This PR leads to the following changes of behavior: 1. The reaction on the `unlabeled`-event is reduced to the case of the last label 2. The `unlabeled`-event will not lead to a rejection of the removal any more 3. Instead of a warning comment a hint comment is posted. We found a way to test the bot a little more realistically (see soehms#11). Additional problems emerged and were also resolved. This led to the following changes in the workflow: 4. A bug in `actor_valid` is fixed (see soehms#10 (comment)) 5. Preparing to use our own bot user (e.g. [sagemathadmins](https://github.com/sagemathadmins)) (see soehms#11 (comment)) 6. Waiver of observing the `reviewDecision` feature provided by GitHub (see 2. in soehms#11 (comment)) 7. Allow the user to revert his decision of label selection (see soehms#11 (comment)) 8. Don't reject label addition any more, except in the case where the author tries to set `s: positive review` to his own PR which has no reviews from others (see soehms#11 (comment)) 9. Dismiss stale reviews of the bot after a push to the branch and on submission of a new review which is more than just a comment (see soehms#11 (comment)). Despite the fact that the testing was now more realistic, it is still not guaranteed that the bot's behavior in `sagemath/sage` will be completely covered by our testing. The changes in this PR cannot be activated immediately after merging into the develop branch due to a bug in the GitHub web interface observed during testing in soehms#11 (see soehms#11 (comment) and soehms#11 (comment)). The problem is that the panel at the top right of the webpage that contains the labels does not update immediately after the bot changes the labels. Since this might cause confusion, I'll create a bug report about it. The going-live of the sync bot will be stalled at least until we received an answer. This PR changes the following flowcharts from sagemath#35172 : ##### What happens when adding `s: needs review` to a PR? ```mermaid --- title: add the needs review label --- flowchart LR %% vertices trigger([label\n 's: needs review'\n added]) mark_as_ready([mark as\n ready for\n review]) remove_other_labels([remove other\n state labels]) warning_about_label_addition([warning\n about label\n addition]) needs_review_valid[[needs review\n valid?]] is_draft{is PR\n a draft?} %% edges trigger --> needs_review_valid needs_review_valid -- true ---> remove_other_labels needs_review_valid -- false ---> is_draft is_draft -- yes ---> mark_as_ready is_draft -- no ---> warning_about_label_addition mark_as_ready --> remove_other_labels ``` The warning is posted as a comment which will be deleted after 5 minutes. ```mermaid --- title: needs review valid? --- flowchart LR %% vertices needs_review_valid([needs review\n valid?]) true([true]) false([false]) latest_review_by_actor{latest review\n by actor} needs_work_valid[[needs work\n valid?]] positive_review_valid[[positive review\n valid?]] is_draft{is PR\n a draft?} %% edges needs_review_valid ---> is_draft is_draft -- yes ---> false is_draft -- no ---> latest_review_by_actor latest_review_by_actor -- yes ---> true latest_review_by_actor -- no ---> needs_work_valid needs_work_valid -- true ---> false needs_work_valid -- false ---> positive_review_valid positive_review_valid -- true ---> false positive_review_valid -- false ---> true ``` ```mermaid --- title: needs work valid? --- flowchart LR %% vertices needs_work_valid([needs work\n valid?]) true([true]) false([false]) latest_review_request_changes{latest proper\n review requests\n changes?} %% edges needs_work_valid --> latest_review_request_changes latest_review_request_changes -- yes ---> true latest_review_request_changes -- no ---> false ``` ```mermaid --- title: positive review valid? --- flowchart LR %% vertices positive_review_valid([positive review\n valid?]) true([true]) false([false]) latest_proper_review_approved{latest proper\n review\n approved?} %% edges positive_review_valid --> latest_proper_review_approved latest_proper_review_approved -- yes ---> true latest_proper_review_approved -- no ---> false ``` Here, proper means that the review is more than a comment. ##### What happens when adding s: needs work to a PR? ```mermaid --- title: add the needs work label --- flowchart LR %% vertices trigger([label\n 's: needs work'\n added]) request_changes([request changes]) remove_other_labels([remove other\n state labels]) warning_about_label_addition([warning\n about label\n addition]) needs_work_valid[[needs work\n valid?]] is_draft{is PR\n a draft?} %% edges trigger --> needs_work_valid needs_work_valid -- true ---> remove_other_labels needs_work_valid -- false ---> is_draft is_draft -- yes ---> warning_about_label_addition is_draft -- no ---> request_changes request_changes --> remove_other_labels ``` ##### What happens when adding `s: positive review` to a PR? ```mermaid --- title: add the positive review label --- flowchart LR %% vertices trigger([label\n 's: positive review'\n added]) positive_review_valid[[positive review\n valid?]] approve_pr([approve]) remove_other_labels([remove other\n state labels]) actor_valid[[actor valid?]] approve_allowed[[approve\n allowed?]] warning_about_label_addition([warning\n about label\n addition]) reject_label_addition[[reject\n label\n addition]] %% edges trigger --> positive_review_valid positive_review_valid -- yes ---> remove_other_labels positive_review_valid -- no ---> actor_valid actor_valid -- yes ---> approve_allowed actor_valid -- no ---> reject_label_addition approve_allowed -- yes ---> approve_pr approve_allowed -- no ---> warning_about_label_addition approve_pr --> remove_other_labels ``` The boxes `actor_valid` and `reject_label_addition` are unchanged. ```mermaid --- title: approve allowed? --- flowchart LR %% vertices trigger([approve\n allowed?]) true([true]) false([false]) review_of_others_request_changes{changes\n requested by\n someone\n else exists?} %% edges trigger --> review_of_others_request_changes review_of_others_request_changes -- yes ---> false review_of_others_request_changes -- no ---> true ``` Here, only reviews of someone else are considered which are more recent than any commit. ##### What happens when a state label is added to an issue? ```mermaid --- title: add a state label to an issue --- flowchart LR %% vertices trigger([label added]) warning_about_label_addition([warning\n about label\n addition]) nothing([do nothing]) is_needs_info{is label\n 's: needs info'?} %% edges trigger --> is_needs_info is_needs_info -- yes ---> nothing is_needs_info -- no ---> warning_about_label_addition ``` ##### What happens when removing a state label from a PR? ```mermaid --- title: remove a state label --- flowchart LR %% vertices trigger([label removed]) nothing([do nothing]) hint_about_label_removal([hint\n about label\n removal]) other_state_label{other state\n labels exist?} is_needs_info{is label\n 's: needs info'?} %% edges trigger --> other_state_label other_state_label -- yes ---> nothing other_state_label -- no ---> is_needs_info is_needs_info -- yes ---> nothing is_needs_info -- no ---> hint_about_label_removal ``` The hint is postet as a comment which will be deleted after 5 minutes. ##### What happens when adding a priority label? ```mermaid --- title: add a priority label --- flowchart LR %% vertices trigger([label added]) remove_other_labels([remove other\n priority labels]) %% edges trigger --> remove_other_labels ``` ##### What happens when removing a priority label? ```mermaid --- title: remove a priority label --- flowchart LR %% vertices trigger([label removed]) nothing([do nothing]) hint_about_label_removal([hint\n about label\n removal]) other_prio_label{other priority\n labels exist?} %% edges trigger --> other_prio_label other_prio_label -- yes ---> nothing other_prio_label -- no ---> hint_about_label_removal ``` ##### What happens when a PR is approved? ```mermaid --- title: approve --- flowchart LR %% vertices trigger([approve\n]) select_positive_review(['s: positive review'\n label selected]) nothing([do nothing]) positive_review_valid[[positive review\n valid?]] pending_review_requests{pending\n review requests\n exists?} actor_authorized{is actor\n a member of\n Triage?} %% edges trigger ---> pending_review_requests pending_review_requests -- yes ---> nothing pending_review_requests -- no ---> actor_authorized actor_authorized -- yes ---> positive_review_valid actor_authorized -- no ---> nothing positive_review_valid -- true ---> select_positive_review positive_review_valid -- false ---> nothing ``` ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36292 Reported by: Sebastian Oehms Reviewer(s): Kwankyu Lee
vbraun
pushed a commit
to vbraun/sage
that referenced
this pull request
Oct 29, 2023
sagemathgh-36292: Fix sync labels issues for step 2 going live completion <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> Unfortunately the fix in sagemath#36213 has not been completed properly. There is still a `return` from the `get_review_decision` method which is not `None` where it should be. This is fixed here. Sorry for this unnecessary trouble and my lack of attention! In a next step this PR fixes a bug which has been noticed by an accidentally activation of the `unlabeled` trigger (see sagemath#36292 (comment)). Simultanously the `labeled`-trigger has been disabled. This is an unrealistic setting which was not covered by the code. Explicitely, a state label could not be removed even though other state labels where set (examples sagemath#36128 and sagemath#36020). This PR leads to the following changes of behavior: 1. The reaction on the `unlabeled`-event is reduced to the case of the last label 2. The `unlabeled`-event will not lead to a rejection of the removal any more 3. Instead of a warning comment a hint comment is posted. We found a way to test the bot a little more realistically (see soehms#11). Additional problems emerged and were also resolved. This led to the following changes in the workflow: 4. A bug in `actor_valid` is fixed (see soehms#10 (comment)) 5. Preparing to use our own bot user (e.g. [sagemathadmins](https://github.com/sagemathadmins)) (see soehms#11 (comment)) 6. Waiver of observing the `reviewDecision` feature provided by GitHub (see 2. in soehms#11 (comment)) 7. Allow the user to revert his decision of label selection (see soehms#11 (comment)) 8. Don't reject label addition any more, except in the case where the author tries to set `s: positive review` to his own PR which has no reviews from others (see soehms#11 (comment)) 9. Dismiss stale reviews of the bot after a push to the branch and on submission of a new review which is more than just a comment (see soehms#11 (comment)). Despite the fact that the testing was now more realistic, it is still not guaranteed that the bot's behavior in `sagemath/sage` will be completely covered by our testing. The changes in this PR cannot be activated immediately after merging into the develop branch due to a bug in the GitHub web interface observed during testing in soehms#11 (see soehms#11 (comment) and soehms#11 (comment)). The problem is that the panel at the top right of the webpage that contains the labels does not update immediately after the bot changes the labels. Since this might cause confusion, I'll create a bug report about it. The going-live of the sync bot will be stalled at least until we received an answer. This PR changes the following flowcharts from sagemath#35172 : ##### What happens when adding `s: needs review` to a PR? ```mermaid --- title: add the needs review label --- flowchart LR %% vertices trigger([label\n 's: needs review'\n added]) mark_as_ready([mark as\n ready for\n review]) remove_other_labels([remove other\n state labels]) warning_about_label_addition([warning\n about label\n addition]) needs_review_valid[[needs review\n valid?]] is_draft{is PR\n a draft?} %% edges trigger --> needs_review_valid needs_review_valid -- true ---> remove_other_labels needs_review_valid -- false ---> is_draft is_draft -- yes ---> mark_as_ready is_draft -- no ---> warning_about_label_addition mark_as_ready --> remove_other_labels ``` The warning is posted as a comment which will be deleted after 5 minutes. ```mermaid --- title: needs review valid? --- flowchart LR %% vertices needs_review_valid([needs review\n valid?]) true([true]) false([false]) latest_review_by_actor{latest review\n by actor} needs_work_valid[[needs work\n valid?]] positive_review_valid[[positive review\n valid?]] is_draft{is PR\n a draft?} %% edges needs_review_valid ---> is_draft is_draft -- yes ---> false is_draft -- no ---> latest_review_by_actor latest_review_by_actor -- yes ---> true latest_review_by_actor -- no ---> needs_work_valid needs_work_valid -- true ---> false needs_work_valid -- false ---> positive_review_valid positive_review_valid -- true ---> false positive_review_valid -- false ---> true ``` ```mermaid --- title: needs work valid? --- flowchart LR %% vertices needs_work_valid([needs work\n valid?]) true([true]) false([false]) latest_review_request_changes{latest proper\n review requests\n changes?} %% edges needs_work_valid --> latest_review_request_changes latest_review_request_changes -- yes ---> true latest_review_request_changes -- no ---> false ``` ```mermaid --- title: positive review valid? --- flowchart LR %% vertices positive_review_valid([positive review\n valid?]) true([true]) false([false]) latest_proper_review_approved{latest proper\n review\n approved?} %% edges positive_review_valid --> latest_proper_review_approved latest_proper_review_approved -- yes ---> true latest_proper_review_approved -- no ---> false ``` Here, proper means that the review is more than a comment. ##### What happens when adding s: needs work to a PR? ```mermaid --- title: add the needs work label --- flowchart LR %% vertices trigger([label\n 's: needs work'\n added]) request_changes([request changes]) remove_other_labels([remove other\n state labels]) warning_about_label_addition([warning\n about label\n addition]) needs_work_valid[[needs work\n valid?]] is_draft{is PR\n a draft?} %% edges trigger --> needs_work_valid needs_work_valid -- true ---> remove_other_labels needs_work_valid -- false ---> is_draft is_draft -- yes ---> warning_about_label_addition is_draft -- no ---> request_changes request_changes --> remove_other_labels ``` ##### What happens when adding `s: positive review` to a PR? ```mermaid --- title: add the positive review label --- flowchart LR %% vertices trigger([label\n 's: positive review'\n added]) positive_review_valid[[positive review\n valid?]] approve_pr([approve]) remove_other_labels([remove other\n state labels]) actor_valid[[actor valid?]] approve_allowed[[approve\n allowed?]] warning_about_label_addition([warning\n about label\n addition]) reject_label_addition[[reject\n label\n addition]] %% edges trigger --> positive_review_valid positive_review_valid -- yes ---> remove_other_labels positive_review_valid -- no ---> actor_valid actor_valid -- yes ---> approve_allowed actor_valid -- no ---> reject_label_addition approve_allowed -- yes ---> approve_pr approve_allowed -- no ---> warning_about_label_addition approve_pr --> remove_other_labels ``` The boxes `actor_valid` and `reject_label_addition` are unchanged. ```mermaid --- title: approve allowed? --- flowchart LR %% vertices trigger([approve\n allowed?]) true([true]) false([false]) review_of_others_request_changes{changes\n requested by\n someone\n else exists?} %% edges trigger --> review_of_others_request_changes review_of_others_request_changes -- yes ---> false review_of_others_request_changes -- no ---> true ``` Here, only reviews of someone else are considered which are more recent than any commit. ##### What happens when a state label is added to an issue? ```mermaid --- title: add a state label to an issue --- flowchart LR %% vertices trigger([label added]) warning_about_label_addition([warning\n about label\n addition]) nothing([do nothing]) is_needs_info{is label\n 's: needs info'?} %% edges trigger --> is_needs_info is_needs_info -- yes ---> nothing is_needs_info -- no ---> warning_about_label_addition ``` ##### What happens when removing a state label from a PR? ```mermaid --- title: remove a state label --- flowchart LR %% vertices trigger([label removed]) nothing([do nothing]) hint_about_label_removal([hint\n about label\n removal]) other_state_label{other state\n labels exist?} is_needs_info{is label\n 's: needs info'?} %% edges trigger --> other_state_label other_state_label -- yes ---> nothing other_state_label -- no ---> is_needs_info is_needs_info -- yes ---> nothing is_needs_info -- no ---> hint_about_label_removal ``` The hint is postet as a comment which will be deleted after 5 minutes. ##### What happens when adding a priority label? ```mermaid --- title: add a priority label --- flowchart LR %% vertices trigger([label added]) remove_other_labels([remove other\n priority labels]) %% edges trigger --> remove_other_labels ``` ##### What happens when removing a priority label? ```mermaid --- title: remove a priority label --- flowchart LR %% vertices trigger([label removed]) nothing([do nothing]) hint_about_label_removal([hint\n about label\n removal]) other_prio_label{other priority\n labels exist?} %% edges trigger --> other_prio_label other_prio_label -- yes ---> nothing other_prio_label -- no ---> hint_about_label_removal ``` ##### What happens when a PR is approved? ```mermaid --- title: approve --- flowchart LR %% vertices trigger([approve\n]) select_positive_review(['s: positive review'\n label selected]) nothing([do nothing]) positive_review_valid[[positive review\n valid?]] pending_review_requests{pending\n review requests\n exists?} actor_authorized{is actor\n a member of\n Triage?} %% edges trigger ---> pending_review_requests pending_review_requests -- yes ---> nothing pending_review_requests -- no ---> actor_authorized actor_authorized -- yes ---> positive_review_valid actor_authorized -- no ---> nothing positive_review_valid -- true ---> select_positive_review positive_review_valid -- false ---> nothing ``` ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36292 Reported by: Sebastian Oehms Reviewer(s): Kwankyu Lee
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unfortunately the fix in #36213 has not been completed properly. There is still a
returnfrom theget_review_decisionmethod which is notNonewhere it should be.This is fixed here. Sorry for this unnecessary trouble and my lack of attention!
In a next step this PR fixes a bug which has been noticed by an accidentally activation of the
unlabeledtrigger (see #36292 (comment)). Simultaneously thelabeled-trigger has been disabled. This is an unrealistic setting which was not covered by the code.Explicitly, a state label could not be removed even though other state labels where set (examples #36128 and #36020). This PR leads to the following changes of behavior:
unlabeled-event is reduced to the case of the last labelunlabeled-event will not lead to a rejection of the removal any moreWe found a way to test the bot a little more realistically (see soehms#11). Additional problems emerged and were also resolved. This led to the following changes in the workflow:
actor_validis fixed (see Fix method on_label_removal soehms/sage#10 (comment))reviewDecisionfeature provided by GitHub (see 2. in Approve without body soehms/sage#11 (comment))s: positive reviewto his own PR which has no reviews from others (see Approve without body soehms/sage#11 (comment))Despite the fact that the testing was now more realistic, it is still not guaranteed that the bot's behavior in
sagemath/sagewill be completely covered by our testing.The changes in this PR cannot be activated immediately after merging into the develop branch due to a bug in the GitHub web interface observed during testing in soehms#11 (see soehms#11 (comment) and soehms#11 (comment)). The problem is that the panel at the top right of the webpage that contains the labels does not update immediately after the bot changes the labels. Since this might cause confusion, I've created the following bug report about it:
Some answers to these reports can be seen in this sage-devel post.
Flowcharts from #35172 changed in this PR
What happens when adding
s: needs reviewto a PR?--- title: add the needs review label --- flowchart LR %% vertices trigger([label\n 's: needs review'\n added]) mark_as_ready([mark as\n ready for\n review]) remove_other_labels([remove other\n state labels]) warning_about_label_addition([warning\n about label\n addition]) needs_review_valid[[needs review\n valid?]] is_draft{is PR\n a draft?} %% edges trigger --> needs_review_valid needs_review_valid -- true ---> remove_other_labels needs_review_valid -- false ---> is_draft is_draft -- yes ---> mark_as_ready is_draft -- no ---> warning_about_label_addition mark_as_ready --> remove_other_labelsThe warning is posted as a comment which will be deleted after 5 minutes.
--- title: needs review valid? --- flowchart LR %% vertices needs_review_valid([needs review\n valid?]) true([true]) false([false]) latest_review_by_actor{latest review\n by actor} needs_work_valid[[needs work\n valid?]] positive_review_valid[[positive review\n valid?]] is_draft{is PR\n a draft?} %% edges needs_review_valid ---> is_draft is_draft -- yes ---> false is_draft -- no ---> latest_review_by_actor latest_review_by_actor -- yes ---> true latest_review_by_actor -- no ---> needs_work_valid needs_work_valid -- true ---> false needs_work_valid -- false ---> positive_review_valid positive_review_valid -- true ---> false positive_review_valid -- false ---> true--- title: needs work valid? --- flowchart LR %% vertices needs_work_valid([needs work\n valid?]) true([true]) false([false]) latest_review_request_changes{latest proper\n review requests\n changes?} %% edges needs_work_valid --> latest_review_request_changes latest_review_request_changes -- yes ---> true latest_review_request_changes -- no ---> false--- title: positive review valid? --- flowchart LR %% vertices positive_review_valid([positive review\n valid?]) true([true]) false([false]) latest_proper_review_approved{latest proper\n review\n approved?} %% edges positive_review_valid --> latest_proper_review_approved latest_proper_review_approved -- yes ---> true latest_proper_review_approved -- no ---> falseHere, proper means that the review is more than a comment.
What happens when adding s: needs work to a PR?
--- title: add the needs work label --- flowchart LR %% vertices trigger([label\n 's: needs work'\n added]) request_changes([request changes]) remove_other_labels([remove other\n state labels]) warning_about_label_addition([warning\n about label\n addition]) needs_work_valid[[needs work\n valid?]] is_draft{is PR\n a draft?} %% edges trigger --> needs_work_valid needs_work_valid -- true ---> remove_other_labels needs_work_valid -- false ---> is_draft is_draft -- yes ---> warning_about_label_addition is_draft -- no ---> request_changes request_changes --> remove_other_labelsWhat happens when adding
s: positive reviewto a PR?--- title: add the positive review label --- flowchart LR %% vertices trigger([label\n 's: positive review'\n added]) positive_review_valid[[positive review\n valid?]] approve_pr([approve]) remove_other_labels([remove other\n state labels]) actor_valid[[actor valid?]] approve_allowed[[approve\n allowed?]] warning_about_label_addition([warning\n about label\n addition]) reject_label_addition[[reject\n label\n addition]] %% edges trigger --> positive_review_valid positive_review_valid -- yes ---> remove_other_labels positive_review_valid -- no ---> actor_valid actor_valid -- yes ---> approve_allowed actor_valid -- no ---> reject_label_addition approve_allowed -- yes ---> approve_pr approve_allowed -- no ---> warning_about_label_addition approve_pr --> remove_other_labelsThe boxes
actor_validandreject_label_additionare unchanged.--- title: approve allowed? --- flowchart LR %% vertices trigger([approve\n allowed?]) true([true]) false([false]) review_of_others_request_changes{changes\n requested by\n someone\n else exists?} %% edges trigger --> review_of_others_request_changes review_of_others_request_changes -- yes ---> false review_of_others_request_changes -- no ---> trueHere, only reviews of someone else are considered which are more recent than any commit.
What happens when a state label is added to an issue?
--- title: add a state label to an issue --- flowchart LR %% vertices trigger([label added]) warning_about_label_addition([warning\n about label\n addition]) nothing([do nothing]) is_needs_info{is label\n 's: needs info'?} %% edges trigger --> is_needs_info is_needs_info -- yes ---> nothing is_needs_info -- no ---> warning_about_label_additionWhat happens when removing a state label from a PR?
--- title: remove a state label --- flowchart LR %% vertices trigger([label removed]) nothing([do nothing]) hint_about_label_removal([hint\n about label\n removal]) other_state_label{other state\n labels exist?} is_needs_info{is label\n 's: needs info'?} %% edges trigger --> other_state_label other_state_label -- yes ---> nothing other_state_label -- no ---> is_needs_info is_needs_info -- yes ---> nothing is_needs_info -- no ---> hint_about_label_removalThe hint is postet as a comment which will be deleted after 5 minutes.
What happens when adding a priority label?
--- title: add a priority label --- flowchart LR %% vertices trigger([label added]) remove_other_labels([remove other\n priority labels]) %% edges trigger --> remove_other_labelsWhat happens when removing a priority label?
--- title: remove a priority label --- flowchart LR %% vertices trigger([label removed]) nothing([do nothing]) hint_about_label_removal([hint\n about label\n removal]) other_prio_label{other priority\n labels exist?} %% edges trigger --> other_prio_label other_prio_label -- yes ---> nothing other_prio_label -- no ---> hint_about_label_removalWhat happens when a PR is approved?
--- title: approve --- flowchart LR %% vertices trigger([approve\n]) select_positive_review(['s: positive review'\n label selected]) nothing([do nothing]) positive_review_valid[[positive review\n valid?]] pending_review_requests{pending\n review requests\n exists?} actor_authorized{is actor\n a member of\n Triage?} %% edges trigger ---> pending_review_requests pending_review_requests -- yes ---> nothing pending_review_requests -- no ---> actor_authorized actor_authorized -- yes ---> positive_review_valid actor_authorized -- no ---> nothing positive_review_valid -- true ---> select_positive_review positive_review_valid -- false ---> nothing📝 Checklist
⌛ Dependencies