Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

Enable prow merging based on OWNERS files #3006

Closed
mattfarina opened this issue Dec 12, 2017 · 21 comments · Fixed by kubernetes/test-infra#5914
Closed

Enable prow merging based on OWNERS files #3006

mattfarina opened this issue Dec 12, 2017 · 21 comments · Fixed by kubernetes/test-infra#5914
Assignees

Comments

@mattfarina
Copy link
Contributor

This issue is to track the status of enabling prow so that those in the OWNERS files who are collaborators on the repo can use prow commands to initiate a merge.

cc: @kubernetes/charts-maintainers

@0xmichalis
Copy link

You will need to change maintainers to approvers in your OWNERS files I think.

cc: @cjwagner

@mattfarina
Copy link
Contributor Author

@Kargakis Created a PR for that in #3014

@scottrigby
Copy link
Member

Just merged #3014. prow looks excellent BTW!

@mattfarina
Copy link
Contributor Author

@kubernetes/charts-maintainers We have a detail we need to work out.... do we want to require /lgtm and /approve or just /approve. Any thoughts?

@viglesiasce
Copy link
Contributor

I think just /approve

@mattfarina
Copy link
Contributor Author

@viglesiasce or just /lgtm? That's an option too.

@scottrigby
Copy link
Member

I see that /lgtm adds the label, so that's a functional difference, but is there any semantic difference between the two?

@viglesiasce
Copy link
Contributor

What is the standard if any for other projects? Is there a definition of what /lgtm means vs /approve?

@0xmichalis
Copy link

0xmichalis commented Dec 12, 2017

Yeah, there is no such thing as just /approve. You either /lgtm and approval is implicit if you are listed as an approver, or you need to /approve explicitly.

@viglesiasce
Copy link
Contributor

In that case i say just /lgtm

@0xmichalis
Copy link

/lgtm is meant to be added by a code reviewer and /approve is meant to be added by a code owner as a high level approval for the change.

@scottrigby
Copy link
Member

@Kargakis does /approve add a /lgtm label? I ask because charts README currently specifies this as part of the review process.

@scottrigby
Copy link
Member

Ah gotcha 👍 Can anyone run /lgtm command then?

@0xmichalis
Copy link

@scottrigby /approve should add an approved label.

@0xmichalis
Copy link

/lgtm with implicit approval should also add an approved label.

@0xmichalis
Copy link

Anyone with access to the Kubernetes org can /lgtm and add the lgtm label but the approved label is added based on OWNERS files.

@scottrigby
Copy link
Member

ok so we should also make sure to update that section of the README. Is there a standard k8s project workflow for these prow commands/labels written up somewhere?

@mattfarina
Copy link
Contributor Author

See #3008 For an example of using the commands and adding the labels.

I get it enough to do the test-infra config now.

@0xmichalis
Copy link

0xmichalis commented Dec 12, 2017

Plugin help is served by prow here.

Plugin help specific to charts is here.

@scottrigby
Copy link
Member

PS, I'm loving looking through these self-documenting prow commands 💯

@cblecker
Copy link
Contributor

This is great :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants