New workflow for synchronization of labels #187

soehms · 2023-02-07T17:49:46Z

This PR is continued in the sage repository as sagemath/sage#35172.

This is a first draft to synchronize the labels migrated from Trac selections list (according to issue #117). These are the priority, type and state labels.

At the moment the script takes care that there is just one item present from each list and it sets defaults on issue and PR opening. Furthermore, it reacts on review approval and change request setting according labels automatically. Setting these labels manually is rejected with a warning comment. If needed that can be changed easily to automatically approving or change requesting.

Open problems and questions:

At the moment there is no synchronization between issue lables of selection lists and corresponding labels of associated PRs.
Does it make sense to have state labels on issues? In the current draft I refuse all but 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.
Recently the type labels have been extended to some Trac components (refactoring, performance,...) so that on some migrated tickets it is non unique any more. Shall we drop uniqueness for these in general?

To be able to run some tests I've developed the draft in the master branch of my trac-to-github fork and created all the labels needed there. I don't know what is the best option for others to test it. Maybe merging it into the upstream repo and create the needed labels there?

Some hints for testing:

Add or remove labels to issues and PRs, or open, close, reopen them and watch out for reactions of the github-actions bot:

If this gives unexpected behavior then look at the log-file of the workflow:

I'm not sure if I will have enough time in the next days and weeks. Therefore, any help and continuation on this PR is welcome!

kwankyu · 2023-02-08T03:59:39Z

That approving PR results in adding "positive review" label may be convenient.

I still think so.

For the release manager, one approving review will be the criterion to merge.

It seems he set the criterion to merge as "s: positive review" label.

It would be convenient that the script removes the status label after merge.

soehms · 2023-02-08T07:20:42Z

This is a first draft to synchronize the labels migrated from Trac selections list (according to issue #117). These are the priority, type and state labels.
At the moment the script takes care that there is just one item present from each list and it sets defaults on issue and PR opening.

Sure, at most one item from priority labels and also from status labels. But I think this is up to humans, not the script.

Humans can be mistaken (even mathematicians). If we agree that a script is needed which is triggered on labeled, unlabeled events why not let it supervise that?

From type labels, we should allow many.

Regarding the set of type labels, I agree. But the name type seems misleading to me.

Furthermore, it reacts on review approval and change request setting according labels automatically. Setting these labels manually is rejected with a warning comment. If needed that can be changed easily to automatically approving or change requesting.

We didn't sort this confusion out yet.

Setting labels by humans should be allowed, for sure.

Yes, if it is guaranteed that there is not a positive review label on a PR that is not approved (which should be checked by the script).

That approving PR results in adding "positive review" label may be convenient. For the release manager, one approving review will be the criterion to merge.

At the moment there is no synchronization between issue lables of selection lists and corresponding labels of associated PRs.

Does it make sense to have state labels on issues? In the current draft I refuse all but 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.

How developers use labels will be gradually sorted out as time passes. I think this is up to the developer culture than to the script.

I think labels would be more useful if there are clear rules and more reliable if they are supervised.

kwankyu · 2023-02-08T07:55:12Z

Humans can be mistaken (even mathematicians). If we agree that a script is needed which is triggered on labeled, unlabeled events why not let it supervise that?

Of course, it doesn't hurt. I do not object.

Regarding the set of type labels, I agree. But the name type seems misleading to me.

It indicates the type (kind?) of a PR. It's already baked into the label prefix t: after a long discussion. Unless there is a better alternative, we should live with it.

Yes, if it is guaranteed that there is not a positive review label on a PR that is not approved (which should be checked by the script).

Okay. I agree with that.

I think labels would be more useful if there are clear rules and more reliable if they are supervised.

Yes if the rules are not overly prohibitive. For that, the set of rules should be small.

For ~~Matthias'~~ inspection, perhaps it is good to list the set of rules that you implemented in the script.

tobiasdiez

This already looks very nice! I've a couple of minor suggestions.

Should this PR maybe go directly to the sagemath/sage repo?

.github/workflows/sync_labels.yml

.github/sync_labels.py

tobiasdiez · 2023-02-08T07:54:49Z

.github/sync_labels.py

+        item = sel_list(label)
+        if sel_list is State and self._pr:
+            if item in [State.positive_review, State.needs_work]:
+                self.add_comment('Label *%s* can not be removed. Please use the corresponding functionality of GitHub' % label)


I would still allow people to actively overwrite the label. For example, if the PR creator realizes she still needs to fix something small, then she can simply add a "needs work" label (but you cannot "request changes" on your own PR). Similarly, not every positive review means that it can directly be merged (e.g. second pairs of eyes needed).

I agree. Perhaps we should not think "positive review" == "approval". Adding "positive review" label is one person's opinion. She may be another person who gives approval to the PR.

I would still allow people to actively overwrite the label. For example, if the PR creator realizes she still needs to fix something small, then she can simply add a "needs work" label (but you cannot "request changes" on your own PR). Similarly, not every positive review means that it can directly be merged (e.g. second pairs of eyes needed).

I agree with respect to needs work. Your second point I don't understand: How has second pairs of eyes needed be realized in Trac other than selecting positive review at the end by the second pairs of eyes?

I agree. Perhaps we should not think "positive review" == "approval". Adding "positive review" label is one person's opinion. She may be another person who gives approval to the PR.

As I said above: I think the meaning of positive review should be as closed to the meaning we are used to from Trac. If we leave the meaning to one person's opinion then the meaning is unknown and the label is not only useless but confusing. Then, as suggested before, we better should remove it.

On trac, you can add yourself as reviewer and comment that you agree with the changes, but don't set the ticket to "positive review". Something like this should still be possible on github, i.e. you review a PR positively but don't add the "positive review" label (or remove it if it has been automatically added).

I always allow to add the needs_work label. This adds a review of github-action requesting changes. Adding positive_review I allow in the following cases (not sure if they are strong enought and cover what you meant by second pairs of eyes):

There must be at least one review of a member

There should not be changes requested by some one else

If in addition the actor is different from the author it is allowed

Elsewise, more checks are needed:
a. There must be at least one review of someone else
b. There must be at least one commit of someone else

If all conditions are satisfied the PR will be approved by github-action. Else the label is removed and a comment: Label can not be added. Please use the corresponding functionality of GitHub is posted.

That sounds very reasonable! Concerning "There should not be changes requested by some one else" not sure what happens in the case someone is against the changes but multiple other people give their positive review. But maybe this situation is rare enough to not need special treatment?

What should be possible is: after giving a positive review, remove the label "s: positive review" and add instead the "needs review" label (meaning "I agree with the changes, but someone else should also have a look at this PR").

That sounds very reasonable! Concerning "There should not be changes requested by some one else" not sure what happens in the case someone is against the changes but multiple other people give their positive review. But maybe this situation is rare enough to not need special treatment?

I think such a conflict should be resolved using the GitHub review functionality, since comments on the respective review that request changes are needed.

What should be possible is: after giving a positive review, remove the label "s: positive review" and add instead the "needs review" label (meaning "I agree with the changes, but someone else should also have a look at this PR").

You can always add s: needs review and s: needs info. In both cases the s: positive review will be removed by the bot.

.github/sync_labels.py

kwankyu · 2023-02-08T11:26:32Z

@sagemath/core

We may consider automatic merging and closing a PR that passes all checks and received one approving review.

This would reduce the release manager's work dramatically.

Would this be possible by the script?

roed314 · 2023-02-08T13:27:51Z

@kwankyu I don't think @vbraun wanted to allow automatic merging of PRs into develop, since there are additional tests that need to be run beyond what it done to get a green check from CI (for example, checking that Sage builds on all supported platforms, running long tests at releases, etc).

kwankyu · 2023-02-08T13:40:39Z

Maybe. Perhaps it may be problematic as well that automatic merging makes the "develop" target move too fast.

kwankyu · 2023-02-09T02:45:11Z

Unmarking draft may also trigger adding "needs review" label by the script.

soehms · 2023-02-09T07:00:43Z

Yes if the rules are not overly prohibitive. For that, the set of rules should be small.

The goal is that the rules are exactly what we are used to from Trac. In Trac you could not have positive review and needs work at the same time. In GitHub this is possible. I don't think that this is overly prohibitive. I admit that the implementation so far does not completely correspond to this goal and of course there are difficulties due to the different environment.

For ~~Matthias'~~ inspection, perhaps it is good to list the set of rules that you implemented in the script.

I've summarized them in the second paragraph of the PR header.

soehms · 2023-02-09T07:04:34Z

This already looks very nice! I've a couple of minor suggestions.

Should this PR maybe go directly to the sagemath/sage repo?

I think that would make sense. How can we test it there? Must the yml- file be in the development -branch (of the fork)?
Or are there other possibilities?

soehms · 2023-02-09T07:33:29Z

Unmarking draft may also trigger adding "needs review" label by the script.

Of course! Good point! I haven't figured out how this is triggered, right now.

kwankyu · 2023-02-09T07:37:12Z

The goal is that the rules are exactly what we are used to from Trac.

I am not sure about that. I think we are passing through the period in which people test somewhat varied workflows in the new environment. I wish that the script helps them conveniently work with the new workflow they would settle on.

In Trac you could not have positive review and needs work at the same time. In GitHub this is possible.

I already agreed that there should be at most one of status labels and priority labels, respectively.

I've summarized them in the second paragraph of the PR header.

Thanks.

tobiasdiez · 2023-02-09T08:57:55Z

This already looks very nice! I've a couple of minor suggestions.
Should this PR maybe go directly to the sagemath/sage repo?

I think that would make sense. How can we test it there? Must the yml- file be in the development -branch (of the fork)? Or are there other possibilities?

It has to be in the default branch of your fork. So simply push this PR to a branch in your sage fork and can change the default branch to it at https://github.com/soehms/sage/settings/branches.

soehms · 2023-02-10T18:46:49Z

The goal is that the rules are exactly what we are used to from Trac.

I am not sure about that.

I mean ideally. Of course that is not achievable.

I think we are passing through the period in which people test somewhat varied workflows in the new environment. I wish that the script helps them conveniently work with the new workflow they would settle on.

I agree! My concern is to keep the priority and state labels reliable.

soehms · 2023-02-10T18:48:09Z

This already looks very nice! I've a couple of minor suggestions.
Should this PR maybe go directly to the sagemath/sage repo?

I think that would make sense. How can we test it there? Must the yml- file be in the development -branch (of the fork)? Or are there other possibilities?

It has to be in the default branch of your fork. So simply push this PR to a branch in your sage fork and can change the default branch to it at https://github.com/soehms/sage/settings/branches.

Thanks! I hope I can find time next week to do the suggested changes and move it to the sage repository.

kwankyu · 2023-02-12T03:39:44Z

I think status labels seems right, according to ChatGPT:

"Status" and "state" are related concepts, but they are not identical. The words are often used interchangeably, but there are subtle differences in meaning and usage between them.

"Status" refers to the condition, position, or situation of a person, thing, or organization. It is often used to describe the level or degree of achievement or progress, such as social status, financial status, or professional status.

"State", on the other hand, refers to the current circumstances, conditions, or situation of something. It is often used to describe the current condition or set of conditions that exist or exist at a particular time. In a broader sense, "state" can also refer to a political entity, such as a nation or a government.

In summary, "status" tends to focus on the level or degree of something, while "state" focuses on the current condition or situation of something.

though I am still not sure.

soehms · 2023-02-13T07:21:59Z

I think status labels seems right, according to ChatGPT:

"Status" and "state" are related concepts, but they are not identical. The words are often used interchangeably, but there are subtle differences in meaning and usage between them.
"Status" refers to the condition, position, or situation of a person, thing, or organization. It is often used to describe the level or degree of achievement or progress, such as social status, financial status, or professional status.
"State", on the other hand, refers to the current circumstances, conditions, or situation of something. It is often used to describe the current condition or set of conditions that exist or exist at a particular time. In a broader sense, "state" can also refer to a political entity, such as a nation or a government.
In summary, "status" tends to focus on the level or degree of something, while "state" focuses on the current condition or situation of something.

though I am still not sure.

Indeed, first I used status, since this is the term used in Trac and in German the Latin version is also preferred against Zustand (translation of state) in such a context. But than I noted that Github uses state for it. In the migration the translation happens here: issue_state, label = map_status(status); state, label = map_status(newvalue); .... I hope this explanation is more useful as that of ChatGPT.

kwankyu · 2023-02-22T00:38:12Z

That approving PR results in adding "positive review" label may be convenient.

I still think so.

For the release manager, one approving review will be the criterion to merge.

It seems he set the criterion to merge as "s: positive review" label.

It would be convenient that the script removes the status label after merge.

soehms · 2023-02-22T07:17:29Z

This already looks very nice! I've a couple of minor suggestions.
Should this PR maybe go directly to the sagemath/sage repo?

I think that would make sense. How can we test it there? Must the yml- file be in the development -branch (of the fork)? Or are there other possibilities?

It has to be in the default branch of your fork. So simply push this PR to a branch in your sage fork and can change the default branch to it at https://github.com/soehms/sage/settings/branches.

Thanks! I hope I can find time next week to do the suggested changes and move it to the sage repository.

I still push my suggestions concerning your review here, I will open a corresponding PR on the sage repository later on with identical branch. Please make your further reviews there.

I will comment on the current changes in the according reviews.

soehms · 2023-02-22T07:24:08Z

That approving PR results in adding "positive review" label may be convenient.

I still think so.

For the release manager, one approving review will be the criterion to merge.

It seems he set the criterion to merge as "s: positive review" label.

It would be convenient that the script removes the status label after merge.

See my comments in the according reviews. This is still just a base of discussion.

soehms · 2023-02-22T18:03:00Z

This already looks very nice! I've a couple of minor suggestions.
Should this PR maybe go directly to the sagemath/sage repo?

I think that would make sense. How can we test it there? Must the yml- file be in the development -branch (of the fork)? Or are there other possibilities?

It has to be in the default branch of your fork. So simply push this PR to a branch in your sage fork and can change the default branch to it at https://github.com/soehms/sage/settings/branches.

Thanks! I hope I can find time next week to do the suggested changes and move it to the sage repository.

I still push my suggestions concerning your review here, I will open a corresponding PR on the sage repository later on with identical branch. Please make your further reviews there.

This is sagemath/sage#35172

soehms · 2023-03-10T21:52:46Z

I close this since it is replaced by sagemath/sage#35172.

soehms and others added 8 commits February 1, 2023 23:42

sync_labels initial

3fb0240

remove unused labels

e4f6d3b

adapt the defaults

c482480

Merge branch 'master' of github.com:soehms/trac-to-github

1cbd42f

use label list via gh view

a0e0753

Merge branch 'sagemath:master' into master

56974b5

add closed and reopened events

525cf7a

Merge branch 'master' of github.com:sagemath/trac-to-github

d29b3e6

soehms changed the title ~~New workflow for synchronization of labels according to #117~~ New workflow for synchronization of labels according to Feb 7, 2023

soehms changed the title ~~New workflow for synchronization of labels according to~~ New workflow for synchronization of labels Feb 7, 2023

soehms mentioned this pull request Feb 7, 2023

Review components and keywords, define labels for future use #99

Open

tobiasdiez reviewed Feb 8, 2023

View reviewed changes

.github/sync_labels.py Outdated Show resolved Hide resolved

Changes according to review

009fcce

soehms mentioned this pull request Feb 22, 2023

New workflow for synchronization of labels sagemath/sage#35172

Merged

4 tasks

soehms closed this Mar 10, 2023

soehms mentioned this pull request Sep 8, 2023

fix and activate pycodestyle E305 in py files sagemath/sage#36177

Merged

2 tasks

soehms mentioned this pull request Sep 28, 2023

Approve without body soehms/sage#11

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New workflow for synchronization of labels #187

New workflow for synchronization of labels #187

soehms commented Feb 7, 2023 •

edited

Loading

kwankyu commented Feb 8, 2023 •

edited

Loading

soehms commented Feb 8, 2023

kwankyu commented Feb 8, 2023 •

edited

Loading

tobiasdiez left a comment

tobiasdiez Feb 8, 2023

kwankyu Feb 8, 2023

soehms Feb 9, 2023

soehms Feb 9, 2023

tobiasdiez Feb 9, 2023

soehms Feb 22, 2023

tobiasdiez Feb 22, 2023

tobiasdiez Feb 22, 2023

soehms Feb 22, 2023

soehms Feb 22, 2023

kwankyu commented Feb 8, 2023 •

edited

Loading

roed314 commented Feb 8, 2023

kwankyu commented Feb 8, 2023 •

edited

Loading

kwankyu commented Feb 9, 2023

soehms commented Feb 9, 2023

soehms commented Feb 9, 2023

soehms commented Feb 9, 2023

kwankyu commented Feb 9, 2023 •

edited

Loading

tobiasdiez commented Feb 9, 2023

soehms commented Feb 10, 2023

soehms commented Feb 10, 2023

kwankyu commented Feb 12, 2023 •

edited

Loading

soehms commented Feb 13, 2023

kwankyu commented Feb 22, 2023

soehms commented Feb 22, 2023

soehms commented Feb 22, 2023

soehms commented Feb 22, 2023

soehms commented Mar 10, 2023

New workflow for synchronization of labels #187

New workflow for synchronization of labels #187

Conversation

soehms commented Feb 7, 2023 • edited Loading

kwankyu commented Feb 8, 2023 • edited Loading

soehms commented Feb 8, 2023

kwankyu commented Feb 8, 2023 • edited Loading

tobiasdiez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwankyu commented Feb 8, 2023 • edited Loading

roed314 commented Feb 8, 2023

kwankyu commented Feb 8, 2023 • edited Loading

kwankyu commented Feb 9, 2023

soehms commented Feb 9, 2023

soehms commented Feb 9, 2023

soehms commented Feb 9, 2023

kwankyu commented Feb 9, 2023 • edited Loading

tobiasdiez commented Feb 9, 2023

soehms commented Feb 10, 2023

soehms commented Feb 10, 2023

kwankyu commented Feb 12, 2023 • edited Loading

soehms commented Feb 13, 2023

kwankyu commented Feb 22, 2023

soehms commented Feb 22, 2023

soehms commented Feb 22, 2023

soehms commented Feb 22, 2023

soehms commented Mar 10, 2023

soehms commented Feb 7, 2023 •

edited

Loading

kwankyu commented Feb 8, 2023 •

edited

Loading

kwankyu commented Feb 8, 2023 •

edited

Loading

kwankyu commented Feb 8, 2023 •

edited

Loading

kwankyu commented Feb 8, 2023 •

edited

Loading

kwankyu commented Feb 9, 2023 •

edited

Loading

kwankyu commented Feb 12, 2023 •

edited

Loading