/approve label should be removed when new changes are detected about sefkhet-abwy

[codificat]

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.

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]

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]

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]

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]

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: - a ‘approved’ `GitHub review <https://help.github.com/articles/about-pull-request-reviews/>`__ must exist - the ‘approved’ `label <https://help.github.com/articles/about-labels/>`__ must be set

[codificat]

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]

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

[goern]

this is solved, isnt it?

[VannTen]

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

[sesheta]

@VannTen: Closing this issue.

In response to this:

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

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.