Going live of sync_labels

[soehms]

Problem Description

Now, it's five months ago that the migration from Trac to GitHub happened. Many of us became familiar with the new workflows and probably found their own ways to use them. From this point of view it seems a little bit late to introduce a new feature to make the workflow more convenient. Convenience isn't the sole purpose of label sync, however. It will also keep our PR's and issues in an unambiguous state.

The GitHub bot that will supervise our state and priority labels has been implemented in https://github.com/sagemath/sage/pull/35172, https://github.com/sagemath/sage/pull/36213 and https://github.com/sagemath/sage/pull/36292 and has already been merged into the develop branch. At the moment the bot is completely disabled in order not to confuse you with a sudden change in behavior when working on PRs and issues. Our plan is to activate it in several steps. Each step can be one or a couple of GitHub triggers the bot will start to listen to. The following events are possible:

Active triggers

• [x] opening of a PR / issue, bot reaction
• [x] reopening of a PR / issue, bot reaction
• [x] closing of a PR / issue, bot reaction
• [ ] labelling a PR / issue, bot reaction
• [ ] unlabelling a PR / issue, bot reaction
• [x] marking a draft PR as ready for review, bot reaction
• [x] converting a PR to a draft, bot reaction
• [ ] submitting a review to a PR, bot reaction: request changes, approve PR
• [x] synchronizing the branch of the PR (i.e. pushing a new commit or rebasing/merging with develop branch), bot reaction

The idea is that the bot's current activity status is visible in the checkboxes above. Each change in the activity of the bot will be scheduled and announced a few days before here.

Proposed Solution

Schedule

step	date	triggers	explanation
1	Tuesday July 18th (2023)	closed	https://github.com/sagemath/sage/issues/35927#issuecomment-1628306377
2	stalled	labeled	https://github.com/sagemath/sage/issues/35927#issuecomment-1664313362
3	Tuesday June 11th (2024)	opened, reopened, converted_to_draft	https://github.com/sagemath/sage/issues/35927#issuecomment-2154192483
4	Tuesday June 18 th (2024)	ready_for_review, synchronize	https://github.com/sagemath/sage/issues/35927#issuecomment-2154204573

Currently progress is blocked because of a bug in the GitHub web-interface. For more information see this post on sage-devel or according remarks in the header of https://github.com/sagemath/sage/pull/36292. For the current status on this see https://github.com/sagemath/sage/issues/35927#issuecomment-2152993322.

Alternatives Considered

If the bot causes you trouble, please report it here. If we can't find an acceptable workaround soon, the bot (or the particular event causing the problem) will be disabled.

If you want to make suggestions for the next steps to be activated, please post them here, too.

Additional Information

No response

Is there an existing issue for this?

• [X] I have searched the existing issues for a bug report that matches the one I want to file, without success.

[soehms]

First step

After the first step is activated the bot will remove all state labels of a PR if it is closed:

---
title: close a PR
---
flowchart LR

%% vertices

    trigger([close PR])
    remove_all_labels([remove all\n state labels])

%% edges

    trigger ---> remove_all_labels

@sagemath/triage

The step will be activated on: Tuesday July 18th

For more information see the header of the issue https://github.com/sagemath/sage/issues/35927#issue-1796027954

[soehms]

Second step

After the second step is activated, when a state label is added, the bot removes all other state labels of a PR. The same applies to priority labels, in this case also for the proper issues.

Additionally, if a status label is added, the review status of the PR will automatically be set accordingly unless previously manually set. The details can be seen in the flowcharts below.

What happens when adding s: needs review to 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_labels

The warning is postet 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 \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 \n review\n approved?}

%% edges

    positive_review_valid  --> latest_proper_review_approved
    latest_proper_review_approved -- yes ---> true
    latest_proper_review_approved -- no ---> false

Reviews older than the most recent commit are not considered!

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_labels

What happens when adding s: positive review to 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_labels

---
title: actor valid
---
flowchart LR

%% vertices

    actor_valid([actor valid?])
    true([true])
    false([false])
    actor_is_author{actor is\n author?}
    other_reviews{reviews of\n someone else\n exists?}
    other_commits{commits of\n someone else\n exists?}

%% edges

    actor_valid  --> actor_is_author
    actor_is_author -- yes ---> other_reviews
    actor_is_author -- no ---> true
    other_reviews -- yes ---> other_commits
    other_reviews -- no ---> false
    other_commits -- yes ---> true
    other_commits -- no ---> false

---
title: reject label addition
---
flowchart LR

%% vertices

    reject_label_addition([reject\n label\n addition])
    add_warning_comment([add\n warning\n comment])
    remove_label([remove label])

%% edges

    reject_label_addition  --> add_warning_comment
    add_warning_comment  --> remove_label

---
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 ar

What happens when adding s: needs info?

---
title: add the needs info label
---
flowchart LR

%% vertices

    trigger([label added])
    remove_other_labels([remove other\n state labels])

%% edges

    trigger --> remove_other_labels

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_addition

The step has been firstly activated on: Saturday September 2nd but deactivated again on September 3nd because of a bug fixed in https://github.com/sagemath/sage/pull/36213 (see https://github.com/sagemath/sage/issues/35927#issuecomment-1704023077 below). A second attempt to enable the trigger on September 16th had to be withdrawn on 17th by issues fixed in https://github.com/sagemath/sage/pull/36292 and merged into 10.2.beta7.

However, this step cannot be reactivated immediately due to a bug in the GitHub web interface I've reported here. For more information see this post on sage-devel or according remarks in the header of https://github.com/sagemath/sage/pull/36292.

[tobiasdiez]

I can do this, but will not have reliable internet until August 23 or so. So maybe just announce it for 'some date after x' instead of 'at x'

[soehms]

I can do this, but will not have reliable internet until August 23 or so. So maybe just announce it for 'some date after x' instead of 'at x'

Thanks very much! I think it's not a problem that the event activation is delayed. However, should unexpected problems arise, it may be necessary to disable the feature again immediately. So if you do this before August 23rd, it would be good to know if in such a case someone else from the @sagemath/core team can step in.

If not, you should rather do it afterwards, even though I'll be on vacation than and mostly offline for two or three weeks (but there is no rush to troubleshoot and fix if deactivation was required).

[tobiasdiez]

I've now activated the "labeled" trigger. If there are any issues, let me know and I will deactivate it again asap.

[soehms]

I've now activated the "labeled" trigger. If there are any issues, let me know and I will deactivate it again asap.

Many Thanks!

[tobiasdiez]

Disabled labeled again now due to issues with positive reviews by non-admins. See https://github.com/sagemath/sage/pull/36177#issuecomment-1704022893 for details.

[soehms]

Status of the GitHub web-interface bug June 2024

The bug in the GitHub web interface that blocks progress in enabling label sync is still not fixed. I ran a new test at https://github.com/soehms/sage/issues/12:

As you can see in the screenshot, the bot removed p:critical, but it is still visible in the top right panel. I've created two bug reports on this and have lost hope that GitHub will fix it.

As argued in this sage-devel post, it would be helpful for new developers who are not members of the Triage team to have their PRs given status labels so that they are better visible to potential reviewers. This could be achieved by enabling the following events for label sync:

• opening of a PR / issue
• reopening of a PR / issue
• marking a draft PR as ready for review
• converting a PR to a draft
• synchronizing the branch of the PR (i.e. pushing a new commit or rebasing/merging with develop branch)

I think these would be less critical in terms of the web interface bug than it is for the labeled event, because the user usually doesn't pay much attention to the panel when triggering one of these events. Moreover, if he expects a reaction on the panel when it's missing, he will intuitively refresh the page and then see it.

For this reason, I suggest continuing the going-live of the bot step by step with the events listed.

[mkoeppe]

Thanks a lot for the update, and for sharing this analysis. I fully agree.

I suggest continuing the going-live of the bot step by step with the events listed.

I support this, and I think the discussion in https://groups.google.com/g/sage-devel/c/sulCa-6EZRA/m/86jFAw9NAAAJ also supported this.

So let's go ahead, what needs to be done?

[soehms]

Thanks a lot for the update, and for sharing this analysis. I fully agree.

I suggest continuing the going-live of the bot step by step with the events listed.

I support this, and I think the discussion in https://groups.google.com/g/sage-devel/c/sulCa-6EZRA/m/86jFAw9NAAAJ also supported this.

So let's go ahead, what needs to be done?

The easy part is enabling the event: you just need to remove the corresponding events from the repository variable SYNC_LABELS_IGNORE_EVENTS (see for example https://github.com/sagemath/sage/pull/35172#issuecomment-1628316014). More effort is needed to involve the community in the process. I think we can do this in two steps:

3. "opened", "reopened", "converted_to_draft"

4. "ready_for_review", "synchronize"

I will describe in this issue what behavior change would be expected for each of the two steps. Then we should announce a date when step 3 will be activated and activate it on that date. After a while, when people are used to the new workflow, we do the same with step 4.

[soehms]

Third step

After the third step is activated, the s: needs review label will be automatically set if a non draft PR is opened. Conversely, if a PR is converted to a draft all state labels will be removed. The details can be seen in the flowcharts below.

What happens when a PR is opened?

---
title: open a PR
---
flowchart LR

%% vertices

    trigger([open\n])
    add_needs_review(['s: needs review'\n label added])
    nothing([do nothing])
    is_draft{is PR\n a draft?}

%% edges

    trigger ---> is_draft
    is_draft -- true ---> nothing
    is_draft -- false ---> add_needs_review

What happens when a PR is converted to a draft?

---
title: convert a PR to a draft
---
flowchart LR

%% vertices

    trigger([close, reopen or\n convert to draft])
    remove_all_labels([remove all\n state labels])

%% edges

    trigger ---> remove_all_labels

@sagemath/triage

This step of label sync will be activated on: Tuesday June 11th

For more information see the header of the issue https://github.com/sagemath/sage/issues/35927#issue-1796027954

[soehms]

Fourth step

After the fourth step is activated, the s: needs review label will be automatically set when a new commit is pushed to a PR or a draft PR is marked as ready for review. The details can be seen in the flowcharts below.

What happens when a draft is ready for review or the branch of a PR is synchronized?

---
title: draft ready for review or branch synchronized
---
flowchart LR

%% vertices

    trigger([ready for\n review])
    nothing([do nothing])
    add_needs_review(['s: needs review'\n label selected])
    needs_review_valid[[needs review\n valid?]]

%% edges

    trigger ---> needs_review_valid
    needs_review_valid -- true ---> add_needs_review
    needs_review_valid -- false ---> nothing

---
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 \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 \n review\n approved?}

%% edges

    positive_review_valid  --> latest_proper_review_approved
    latest_proper_review_approved -- yes ---> true
    latest_proper_review_approved -- no ---> false

Reviews older than the most recent commit are not considered!

@sagemath/triage

This step of label sync will be activated on: Tuesday June 18th

For more information see the header of the issue https://github.com/sagemath/sage/issues/35927#issue-1796027954

[mkoeppe]

Sounds good to me, please go ahead

[soehms]

Sounds good to me, please go ahead

Since I can't change the repository variable, I need your help with this step. What date would be OK for you? For me, Tuesday, Thursday, and Friday(?) would be the best dates next week to find time in case any problems arise.

[mkoeppe]

Any day of the week is fine with me and any date this month

[soehms]

Any day of the week is fine with me and any date this month

Then lets start with it next Tuesday!

[soehms]

Any day of the week is fine with me and any date this month

It seems that it is not done, yet: https://github.com/sagemath/sage/actions/runs/9468962760

[mkoeppe]

OK, I've made the change now. Before:

["opened", "reopened", "labeled", "unlabeled", "ready_for_review", "synchronize", "review_requested", "converted_to_draft", "submitted"]

After:

["labeled", "unlabeled", "ready_for_review", "synchronize", "review_requested", "submitted"]

[soehms]

OK, I've made the change now. Before:

["opened", "reopened", "labeled", "unlabeled", "ready_for_review", "synchronize", "review_requested", "converted_to_draft", "submitted"]

After:

["labeled", "unlabeled", "ready_for_review", "synchronize", "review_requested", "submitted"]

Shall we do the next step (for "ready_for_review", "synchronize") next Tuesday?

[mkoeppe]

Works for me

[soehms]

Works for me

Thats today!

[mkoeppe]

I've made the change. It's now

["labeled", "unlabeled", "review_requested", "submitted"]