search

AICoE / sefkhet-abwy

This is one of the incarnations of Sesheta
GNU General Public License v3.0
5 stars 13 forks source link

/approve label should be removed when new changes are detected #204

Closed mayaCostantini closed 1 year ago

mayaCostantini commented 1 year ago

Describe the bug The /approve label should be removed when new changes are detected on a pull request, as for the /lgtm label to prevent accidental merges. An alternative would also be to add a /hold label when new changes are detected after an approval and to ask for an additional review.

Additional context See https://github.com/thoth-station/storages/pull/2674 for an example of PR merge with additional changes after approval.

codificat commented 1 year ago

approve and lgtm are meant to mean slightly different things:

Based on that, I believe that the request here translates to: "require both lgtm and approved labels for merging".

Does that make sense?

Side note: I'm not sure this repo is the place for this request... maybe operate-first/apps where the prow configuration is? Note how e.g. the operate-first and os-climate repos currently require both labels for merging

VannTen commented 1 year ago

On Mon, Oct 10, 2022 at 07:28:40AM -0700, Pep Turró Mauri wrote:

approve and lgtm are meant to mean slightly different things:

  • approved: a maintainer agrees this change should be made. This doesn't change if the code is updated/amended.
  • lgtm: a reviewer agrees the correctly implements the change. This changes if the code changes, that's why this label is removed in that case.

Thanks for the clarification, because it was not clear for me ^

Based on that, I believe that the request here translates to: "require both lgtm and approved labels for merging".

I think that should be the case with that distinction clarified

VannTen commented 1 year ago

Further clarification: /lgtm is what we should do when doing a PR review, right ? The fact that Github call that "approved" could be cause some confusion.

codificat commented 1 year ago

Further clarification: /lgtm is what we should do when doing a PR review, right ?

Correct.

The fact that Github call that "approved" [...]

Hmm... does it? As far as I know, GitHub is not even aware of the implications of /lgtm and other prow commands in the comments, no? It is prow (sesheta) that reacts to them and updates the labels. But for /lgtm it would add the lgtm label, not the approved label...

VannTen commented 1 year ago

On Tue, Oct 11, 2022 at 03:44:02AM -0700, Pep Turró Mauri wrote:

The fact that Github call that "approved" [...]

Hmm... does it? As far as I know, GitHub is not even aware of the implications of /lgtm and other prow commands in the comments, no? It is prow (sesheta) that reacts to them and updates the labels. But for /lgtm it would add the lgtm label, not the approved label...

What I mean is that github has the concept of approval for the review (Request Changes/Comment/Approve) which is disconnected from the label /lgtm and /approved handled by prow/sesheta

And in the docs for code-review in thoth-station/core we currently states that we need the /approved label and a approved review.

Excerpt:

The following requirements must be met to start merging a Pull Request:

codificat commented 1 year ago

the docs for code-review in thoth-station/core we currently states that we need the /approved label and a approved review.

The doc is out of date, then. This does not reflect the current reality, where prow will merge based on its configuration (labels, checks).

Let's wait for the outcome of this issue for the final details, but in any case the code-review document should be updated.

codificat commented 1 year ago

A relevant reference is https://github.com/kubernetes/test-infra/blob/master/prow/plugins/approve/approvers/README.md

goern commented 1 year ago

this is solved, isnt it?

VannTen commented 1 year ago

Yes, fixed by operate-first/apps#2554 /close

sesheta commented 1 year ago

@VannTen: Closing this issue.

In response to [this](https://github.com/AICoE/sefkhet-abwy/issues/204#issuecomment-1282398275): >Yes, fixed by operate-first/apps#2554 >/close > > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.