New workflow for synchronization of labels

[soehms]

This PR is continued in the sage repository as https://github.com/sagemath/sage/pull/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]

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]

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]

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.

[kwankyu]

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

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

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

[kwankyu]

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

[soehms]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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 https://github.com/sagemath/sage/pull/35172

[soehms]

I close this since it is replaced by https://github.com/sagemath/sage/pull/35172.