New workflow for synchronization of labels

[soehms]

📚 Description

This PR is a continuation of sagemath/trac-to-github#187.

It implements GitHub actions to synchronize the labels migrated from Trac selection lists (according to issue sagemath/trac-to-github#117). These are the priority and state labels (type labels are not considered here since their meaning has changed).

The bot will not be active immediatly after this PR is merged. It needs to be activated by a maintainer of the repository by adding the variable SYNC_LABELS_ACTIVE with value yes under

Settings -> Secrets and variables -> Actions

Another repository variable SYNC_LABELS_IGNORE_EVENTS can be used to disable the bot on specific GitHub triggers. For example, SYNC_LABELS_IGNORE_EVENTS = ["unlabeled", "submitted"] causes the bot to suspend responding to these events.

At the moment the script takes care that there is just one item present from each list and it sets s: needs review on a non draft PR opening. Furthermore, it reacts on review approval and change requests setting according labels automatically (after some checks). Conversely, setting a state label will lead to automatic approval resp. change request under certain circumstances. If there is need to reject it a warning comment is posted.

For a detailed description of the workflows see the flowcharts below. In summary, the bot keeps the meaning consistent along the rows in the table below:

Trac status	GH label	closed	draft	review decision
new		No	Yes	None
needs_info	s: needs info	No	No / Yes	Any
needs review	s: needs review	No	No	Any
needs work	s: needs work	No	No	CHANGES REQUESTED
positive review	s: positive review	No	No	APPROVED
closed		Yes	No / Yes	Any

The review decision is not given explicitly on all cases. Implicitly it can be read of the list of all reviews. So if there is at least one review requesting changes younger than any commit the review decision is interpreted as CHANGES REQUESTED. If there is at least one approved review younger than any commit but none requesting changes the review decision is interpreted as APPROVED.

Please consider this implementation just as a base of discussion.

Open problems and questions

• At the moment there is no synchronization between issue labels of selection lists and corresponding labels of associated PRs
• Does it make sense to have state labels on issues? In the current version of the PR I reject all but s: needs info.
• In general the relation between issues and PRs can be n:m. Shall we accept different priorities among them or shall we put all these to the maximum. I think at least the priority of a PR should be the maximum of all issues fixed by it.

Notes on testing

The failure of the Synchronize selection list labeled / synchronize (pull_request) check is to be expected (see #35172 (comment)) since it is trying to download the python file from the upstream repository (sagemath/sage). Therefore testing the script must be done relative to my fork (soehms/sage). First examples are soehms#1 and soehms#2. It would be helpful if others would open PRs there in order to test the review functionality which I can't do by myself.

Flowcharts

What happens when a PR changes its state?

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

Loading

What happens when a PR is closed or reopened or converted to a draft?

---
title: close, reopen or 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

Loading

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

Loading

---
title: needs review valid?
---
flowchart LR

%% vertices

    needs_review_valid([needs review\n valid?])
    true([true])
    false([false])
    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 ---> 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

Loading

---
title: needs work valid?
---
flowchart LR

%% vertices

    needs_work_valid([needs work\n valid?])
    true([true])
    false([false])
    proper_review_exists{proper review\n after most recent\n commit exists?}
    review_decision_exists{review\n decision\n exists?}
    review_decision_request_changes{review\n decision is\n CHANGES\n REQUESTED?}
    any_review_request_changes{any proper\n review requests\n changes?}

%% edges

    needs_work_valid  --> proper_review_exists
    proper_review_exists -- yes ---> review_decision_exists
    proper_review_exists -- no ---> false
    review_decision_exists -- yes ---> review_decision_request_changes
    review_decision_exists -- no ---> any_review_request_changes
    review_decision_request_changes -- yes --> true
    review_decision_request_changes -- no --> false
    any_review_request_changes -- yes ---> true
    any_review_request_changes -- no ---> false

Loading

Here, proper means that the review is more than a comment.

---
title: positive review valid?
---
flowchart LR

%% vertices

    positive_review_valid([positive review\n valid?])
    true([true])
    false([false])
    proper_review_exists{proper review\n after most recent\n commit exists?}
    review_decision_exists{review\n decision\n exists?}
    review_decision_approved{review\n decision is\n APPROVED?}
    all_proper_reviews_approved{all proper\n reviews\n approved?}

%% edges

    positive_review_valid  --> proper_review_exists
    proper_review_exists  -- yes ---> review_decision_exists
    proper_review_exists  -- no ---> false
    review_decision_exists -- yes ---> review_decision_approved
    review_decision_exists -- no ---> all_proper_reviews_approved
    review_decision_approved -- yes --> true
    review_decision_approved -- no --> false
    all_proper_reviews_approved -- yes ---> true
    all_proper_reviews_approved -- no ---> false

Loading

Here, proper means that the review is more than a comment.

What happens when changes are requested for a PR?

---
title: request changes
---
flowchart LR

%% vertices

    trigger([request\n changes\n])
    add_needs_work(['s: needs work'\n label selected])
    nothing([do nothing])
    needs_work_valid[[needs work\n valid?]]

%% edges

    trigger ---> needs_work_valid
    needs_work_valid -- true ---> add_needs_work
    needs_work_valid -- false ---> nothing

Loading

What 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?]]
    actor_authorized{is actor\n a member of\n Triage?}

%% edges

    trigger ---> actor_authorized
    actor_authorized -- yes ---> positive_review_valid
    actor_authorized -- no ---> nothing
    positive_review_valid -- true ---> select_positive_review
    positive_review_valid -- false ---> nothing

Loading

What happens when state or priority labels are changed?

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])
    approve_allowed[[approve\n allowed?]]
    reject_label_addition[[reject\n label\n addition]]

%% edges

    trigger --> positive_review_valid
    positive_review_valid -- yes ---> remove_other_labels
    positive_review_valid -- no ---> approve_allowed
    approve_allowed -- yes ---> approve_pr
    approve_allowed -- no ---> reject_label_addition
    approve_pr --> remove_other_labels

Loading

---
title: approve allowed?
---
flowchart LR

%% vertices

    trigger([approve\n allowed?])
    true([true])
    false([false])
    review_of_member_exists{review\n of a member\n exists?}
    review_of_others_request_changes{changes\n requested by\n someone\n else exists?}
    actor_valid[[actor valid?]]

%% edges

    trigger --> review_of_member_exists
    review_of_member_exists -- yes ---> review_of_others_request_changes
    review_of_member_exists -- no ---> false
    review_of_others_request_changes -- yes ---> false
    review_of_others_request_changes -- no ---> actor_valid
    actor_valid -- yes ---> true
    actor_valid -- no ---> false

Loading

Here, only reviews of someone else are considered which are more recent than any commit.

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

Loading

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

Loading

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])
    reject_label_addition[[reject\n 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 ---> reject_label_addition
    is_draft -- no ---> request_changes
    request_changes --> remove_other_labels

Loading

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])
    reject_label_addition[[reject\n 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 ---> reject_label_addition
    mark_as_ready --> remove_other_labels

Loading

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

Loading

What happens when adding a state label to an issue?

---
title: add a state label to an issue
---
flowchart LR

%% vertices

    trigger([label added])
    reject_label_addition([reject\n 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 --->  reject_label_addition

Loading

What happens when removing a state label from a PR?

---
title: remove a state label
---
flowchart LR

%% vertices

    trigger([label removed])
    nothing([do nothing])
    reject_label_removal([reject\n label\n removal])
    is_needs_info{is label\n 's: needs info'?}

%% edges

    trigger --> is_needs_info
    is_needs_info -- yes --->  nothing
    is_needs_info -- no --->  reject_label_removal

Loading

Nothing happens when a state label is removed from an issue.

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_labels

Loading

What happens when removing a priority label?

---
title: remove a priority label
---
flowchart LR

%% vertices

    trigger([label removed])
    reject_label_removal([reject\n label\n removal])

%% edges

    trigger --> reject_label_removal

Loading

📝 Checklist

• I have made sure that the title is self-explanatory and the description concisely explains the PR.
• I have linked an issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation accordingly.

⌛ Dependencies

[codecov-commenter]

Codecov Report

Patch coverage has no change and project coverage change: -0.02 ⚠️

Comparison is base (c00e6c2) 88.62% compared to head (c829525) 88.60%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #35172      +/-   ##
===========================================
- Coverage    88.62%   88.60%   -0.02%     
===========================================
  Files         2148     2148              
  Lines       398855   398855              
===========================================
- Hits        353480   353418      -62     
- Misses       45375    45437      +62     

see 28 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kwankyu]

At the moment the script takes care that there is just one item present from each list and it sets s: needs review on a non draft PR opening.

PR starts as non-draft. Hence this means a new PR gets "s: needs review" from the start. But an author of a PR may want to see checks reports before he/she adds "s: needs review" or even "s: needs work" for failed checks.

[soehms]

At the moment the script takes care that there is just one item present from each list and it sets s: needs review on a non draft PR opening.

PR starts as non-draft. Hence this means a new PR gets "s: needs review" from the start. But an author of a PR may want to see checks reports before he/she adds "s: needs review" or even "s: needs work" for failed checks.

Whenever I created a PR I had the choice to create it as a draft or ready for review. But I can't say which setting is responsible for that. If a PR has been created as ready for review you can change it to a draft using GH functionality. In this case the s: needs review label will be removed by the bot. Alternatively you can just remove the s: needs review label. In this case the bot will add the s: needs info label. If you remove this, too, then the bot should (consequently) convert the PR to a draft. But this is not implemented, yet.

I've added a table to the PR description, to make this more clear.

[kwankyu]

PR starts as non-draft. Hence this means a new PR gets "s: needs review" from the start. But an author of a PR may want to see checks reports before he/she adds "s: needs review" or even "s: needs work" for failed checks.

Whenever I created a PR I had the choice to create it as a draft or ready for review. But I can't say which setting is responsible for that.

Yes. I have the choice too. I learned that just now (or I knew but forgot). It seems very easy to miss that.

But the situation I described is somewhat different. One may think that his PR is not a draft but needs affirmative checks (github workflows) for sanity or some discussion before proceeding to "needs review". This PR itself seems an example of this situation :-)

I've added a table to the PR description, to make this more clear.

A flowchart of the full development workflow may also help for this discussion.

[kwankyu]

One may think that his PR is not a draft but needs affirmative checks (github workflows) for sanity or some discussion before proceeding to "needs review".

I now agree that the author should set the PR as a draft in this situation.

As the authors who is not in the triage team cannot add labels, the draft/ready-for-review setting is the only available way to express the state of the PR.

I've added a table to the PR description, to make this more clear.

Hence "s: needs review" just reflects the yes/no draft state as the table shows.

[soehms]

As the authors who is not in the triage team cannot add labels, the draft/ready-for-review setting is the only available way to express the state of the PR.

This is one reason (see also the discussion in this sage-devel thread). Another one: If a PR is not a draft it is ready for review by GitHub terminology. So, if we like to avoid confusion between similar concepts (ready for review versus needs review) it's better to synchronize these, too.

This PR itself seems an example of this situation :-)

Yes, accidentally.

A flowchart of the full development workflow may also help for this discussion.

Good idea! Are mermaid code-blocks the tool we use for that?

[kwankyu]

Yes, it works nicely. sequenceDiagram may suit us.

[soehms]

Yes, it works nicely. sequenceDiagram may suit us.

I've added flowcharts to the PR header, since they fit better in this case.

[tobiasdiez]

Wow, very nice! For my taste its a tad to axiomatized, but from my point of view we can try this out and adapt it later once we run into situations where the automatization is to rigid.

The added workflow is failing at the moment though.

[kwankyu]

Perhaps this is already the established manual workflow on GitHub:

sequenceDiagram
participant new PR
participant draft
participant needs review
participant needs info
participant needs work
new PR -->> draft: open as draft
new PR -->> needs review: open
draft -->> needs review: ready for review
needs review -->> draft: draft
needs review ->> needs info: needs info
needs info -->> needs review: ready for review
needs review ->> needs work: Change Requested
needs work -->> needs review: ready for review
needs review ->> positive review: Approved

Loading

Are your diagrams conformant with this?

[soehms]

Perhaps this is already the established manual workflow on GitHub:

Are your diagrams conformant with this?

I would say: Yes!

[tobiasdiez]

I don't have much time at the moment for a proper review, but what I've seen by skimming over this PR it looks good to me.

[soehms]

I don't have much time at the moment for a proper review, but what I've seen by skimming over this PR it looks good to me.

No problem. Anyhow, we should give the community a look at the workflow before we proceed. Also, I just realized I missed a point (see sagemath/trac-to-github#117 (comment)) which should be done before.

[soehms]

Perhaps this is already the established manual workflow on GitHub:
Are your diagrams conformant with this?

I would say: Yes!

But one point is still unclear: The vertical lines in your chart appear to represent the Trac status except the one labeled draft. On the other hand the needs info status is missing. Shall we treat them as synonyms? So far I have considered them to be independent. But maybe things would become easier if we treat the label s: needs info as synonym for a draft PR (although that doesn't have quite the same meaning as we used needs info in Trac).

[kwankyu]

But one point is still unclear: The vertical lines in your chart appear to represent the Trac status except the one labeled draft.

I intended them to represent labels except the draft (with no label)

On the other hand the needs info status is missing. Shall we treat them as synonyms? So far I have considered them to be independent. But maybe things would become easier if we treat the label s: needs info as synonym for a draft PR (although that doesn't have quite the same meaning as we used needs info in Trac).

I added needs info to my chart. The solid line from needs review to needs info is an action of a reviewer, and the dotted line from needs info to needs review is an action of the author. But I don't know what those actions should be. As far as I know, there is no appropriate action(like a button) on the github interface that can be used to trigger the actions...

Could a comment with sole content "needs review" or "needs info" may trigger the actions, by your script?

[kwankyu]

Happy vacation!

[soehms]

Thanks for your hard work and also for being patient with me!

No problem. Everything was absolutely necessary.

[soehms]

Happy vacation!

Many thanks! Also for your review!

[kwankyu]

One feature of the new workflow is to automatically remove "positive review" label (or rather all status labels) from the merged PRs. Right? Then we need this feature as soon as possible. Could we turn this on? I think turning this feature on does not need explicit agreement from sage-devel, but would need an anouncement.

[soehms]

One feature of the new workflow is to automatically remove "positive review" label (or rather all status labels) from the merged PRs. Right? Then we need this feature as soon as possible. Could we turn this on? I think turning this feature on does not need explicit agreement from sage-devel, but would need an anouncement.

According to my #35172 (comment) I've created #35927 to track the going live of the bot. We should fix a date to turn the closed trigger on. @tobiasdiez can you do the job of setting the corresponding variables?

SYNC_LABELS_ACTIVE: yes
SYNC_LABELS_IGNORE_EVENTS: ["opened", "reopened", "labeled", "unlabeled", "ready_for_review", "synchronize", "review_requested", "converted_to_draft", "submitted"]

If yes, what would be a good date?

[kwankyu]

Nice!

So as soon as Tobias set the variables, the bot starts to listen to the "closed" event (only). Right?

Then let's have at least a week before Tobias setting the variables, after you make an announcement in sage-devel.

[tobiasdiez]

Yes, I can do this whenever you want. From my point of view we can also switch it on immediately.

[kwankyu]

I "feel" so with the closed event. But it is safe to go through long and winding roads.

[soehms]

Nice!

So as soon as Tobias set the variables, the bot starts to listen to the "closed" event (only). Right?

Exactly!

[soehms]

I "feel" so with the closed event. But it is safe to go through long and winding roads.

I agree. There will definitely be steps that need to be announced. Therefore, for didactic reasons, it would not make sense to start with an exception.

Will next Tuesday July 18th be OK? What time of day suits you @tobiasdiez ?

BTW: maybe one of you knows about an oddity that happened to my (completely unrelated) PR #35800. It has been merged into the current beta:

commit 8cffff5f66ee7dbab7f0e591c496bf169a60293a
Merge: 8c1b87c383 83ecdfc302
Author: Release Manager <release@sagemath.org>
Date:   Sat Jul 8 12:53:56 2023 +0200

    gh-35800: Adapt the KnotInfo interface to new Khovanov polynomial data

However, it is still open and not marked as merged. I suspect this is caused by a sequence of two shared commits with Travis that I accepted in the web interface, followed by a legacy commit that I pushed (after the pull) from my local repository.

[tobiasdiez]

Will next Tuesday July 18th be OK?

Yes, that should work for me.

[soehms]

Will next Tuesday July 18th be OK?

Yes, that should work for me.

Many thanks! I've just announced the date on #35927 (comment) and sage-devel.

[soehms]

Will next Tuesday July 18th be OK?

Yes, that should work for me.

@tobiasdiez, Did you do it?

[kwankyu]

@dima would you take care of #35172 (comment)? We are not on schedule.

[tobiasdiez]

Sorry for the delay, I've set the variables now.