Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.pullapprove.yml: List project maintainers separately #29

Merged

Conversation

wking
Copy link
Contributor

@wking wking commented Jan 7, 2017

This repository is managed by the union of all OCI Project maintainers. To avoid having a few active maintainers from one project driving the template in ways that other projects don't support, require at least two approvals from each project. This will slow down acceptance a bit, but many maintainers belong to multiple projects (and their votes will count for each group), so you won't need 12 separate maintainers voting to get a single project-template PR across the line.

A side benefit of this is that we no longer need to maintain the tdc-maintainers team.

Also update from config format v1 to v2 and pull in some new features (which I can spin off into separate PRs if they turn out to be controversial).

@wking wking force-pushed the distributed-pull-approve-teams branch 12 times, most recently from f6dbf92 to e4ca9f0 Compare January 7, 2017 14:31
@wking
Copy link
Contributor Author

wking commented Jan 7, 2017

The “.pullapprove.yml proposed by this PR is invalid” error is because each of the teams will need to be assigned to this repository. From the PullApprove page:

Parsing errors

"['go-digest-maintainers']" is an invalid value for teams: go-digest-maintainers team not found on this repo

@jonboulle
Copy link

@caniszczyk can we adopt this? opencontainers/image-spec#609 (comment) is kinda annoying

@caniszczyk
Copy link
Contributor

@jonboulle @wking sure, we also need to enable the "github_reviews" option to ensure that those are counted: https://docs.pullapprove.com/groups/github_reviews/

@wking
Copy link
Contributor Author

wking commented Mar 16, 2017 via email

.pullapprove.yml Outdated
approve_by_comment:
enabled: true
approve_regex: ^LGTM
reject_regex: ^Rejected
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're doing this, can we make this NACK? 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're doing this, can we make this NACK?

My preference would be to use LGTM and REJECT (what does NACK even mean? "Not acknowledged" sounds like "I'm refusing to admit this exists" and not like "I don't like this"). But I don't really care what the terms are as long as they're special enough to not be accidental.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, NACK is how LKML does it. I don't really mind but it would be nice if it was all-caps so that it stands out in the text.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw I always took "nack" as "negative acknowledgement" (I acknowledge reviewing this and it should not be shipped at this time)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw I always took "nack" as "negative acknowledgement"…

Good enough for me. I've spun this off into #32 to avoid distracting from this PR.

.pullapprove.yml Outdated
author_approval:
ignored: true
always_pending:
title_regex: 'WIP'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be ^WIP.

.pullapprove.yml Outdated
required: 2
reject_value: -1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should have this, since it means you'd need 3 LGTMs to get a NACK'd PR merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should have this, since it means you'd need 3 LGTMs to get a NACK'd PR merged.

That's why I put it in ;). It seems like there should be a distinction between abstains/no-review and "I looked at this and didn't like it".

But I'm happy to pull 0a60167 out into a follow-up PR if it would help land the rest of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've spun this point off into #31 so we can land the rest of this PR.

@wking wking force-pushed the distributed-pull-approve-teams branch from 96ab978 to c87ca52 Compare March 17, 2017 04:59
wking added a commit to wking/oci-project-template that referenced this pull request Mar 17, 2017
Docs in http://docs.pullapprove.com/groups/always_pending/

The ^ prefix (anchoring the match to the front of the title) is
Aleksa's suggestion [1].

[1]: opencontainers#29 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Mar 17, 2017 via email

@caniszczyk
Copy link
Contributor

caniszczyk commented Mar 17, 2017

Can you squash this into one commit and rebase this?

wking added a commit to wking/oci-project-template that referenced this pull request Mar 17, 2017
I'm fine with REJECT, but Aleksa suggested a switch to NACK [1] to
match the Linux Kernel's usage [2,3].  Jon suggests it may mean
“negative acknowledgement” [4].

I've updated both GOVERNANCE and the PullApprove config because it
seems better to have consistent keywords (so maintainers don't have to
remember which context-dependent string keyword should use).

[1]: opencontainers#29 (comment)
[2]: opencontainers#29 (comment)
[3]: http://lkml.iu.edu/hypermail/linux/kernel/0706.0/0303.html
[4]: opencontainers#29 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking wking mentioned this pull request Mar 17, 2017
@wking wking force-pushed the distributed-pull-approve-teams branch from 06f8c77 to 7233104 Compare March 17, 2017 16:31
@wking wking force-pushed the distributed-pull-approve-teams branch from 7233104 to 8feb0fd Compare March 17, 2017 16:32
This repository is managed by the union of all OCI Project
maintainers.  To avoid having a few active maintainers from one
project driving the template in ways that other projects don't
support, require at least two approvals from each project.  This will
slow down acceptance a bit, but many maintainers belong to multiple
projects (and their votes will count for each group [1]), so you won't
need 12 separate maintainers voting to get a single project-template
PR across the line.

Also update from config format v1 to v2 [1,2].

[1]: http://docs.pullapprove.com/migrating-to-v2/
[2]: http://docs.pullapprove.com/

Signed-off-by: W. Trevor King <wking@tremily.us>
Docs in http://docs.pullapprove.com/groups/always_pending/

The ^ prefix (anchoring the match to the front of the title) is
Aleksa's suggestion [1].

[1]: opencontainers#29 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
The main example has it outside [1], but it's a group setting [1], so
we can move it inside for consistency [2].

[1]: http://docs.pullapprove.com/
[2]: http://docs.pullapprove.com/pr-settings/group_defaults/

Signed-off-by: W. Trevor King <wking@tremily.us>
This project was initially approved as go-selinux [1] but has since
renamed itself to drop the go- prefix [2].

[1]: https://groups.google.com/a/opencontainers.org/forum/#!topic/tob/nH8QU6UnrqA
     Subject: VOTE Required: approve new project go-selinux
     Date: Fri, 17 Feb 2017 01:21:37 +0000
     Message-ID: <CAD2oYtPzgJKeiKKxFnds-yH+2OG3WF6d36gR1uD2Y3+QQdsgpw@mail.gmail.com>
[2]: opencontainers/selinux#6 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@wking
Copy link
Contributor Author

wking commented Mar 17, 2017

Can you squash and rebase this?

Rebased with 06f8c778feb0fd.

Why do you want it squashed? Each of these changes seem fairly orthogonal. I'm happy to squash 8feb0fd into f412693, but otherwise I think git blame … is more useful with smaller commits.

wking added a commit to wking/oci-project-template that referenced this pull request Mar 17, 2017
Catching up with opencontainers/tob@0057edc (Merge pull request opencontainers#29
from opencontainers/selinux-project, 2017-02-23) and the subsequent
rename [1].

I'd still rather drop the parenthetical entirely and link to a place
that listed OCI Projects, but we don't have a canonical target for
that yet (opencontainers/tob#2) and the current closest instance seems
to be the GitHub section in [2] (which doesn't have the "OCI Project"
words).

[1]: opencontainers/selinux#6 (comment)
[2]: https://www.opencontainers.org/community

Signed-off-by: W. Trevor King <wking@tremily.us>
@caniszczyk
Copy link
Contributor

@wking it's a preference thing but I don't have energy right now to fight the request

@caniszczyk caniszczyk merged commit cea6619 into opencontainers:master Mar 17, 2017
@wking
Copy link
Contributor Author

wking commented Mar 17, 2017

Looks like we're still missing an selinux-maintainers group.

@wking wking deleted the distributed-pull-approve-teams branch March 17, 2017 17:08
wking added a commit to wking/oci-project-template that referenced this pull request Mar 17, 2017
I'm fine with REJECT, but Aleksa suggested a switch to NACK [1] to
match the Linux Kernel's usage [2,3].  Jon suggests it may mean
“negative acknowledgement” [4].

I've updated both GOVERNANCE and the PullApprove config because it
seems better to have consistent keywords (so maintainers don't have to
remember which context-dependent string keyword should use).

[1]: opencontainers#29 (comment)
[2]: opencontainers#29 (comment)
[3]: http://lkml.iu.edu/hypermail/linux/kernel/0706.0/0303.html
[4]: opencontainers#29 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/image-spec that referenced this pull request Mar 17, 2017
Pull in changes from [1].  The only changes vs. the upstream version
are:

* Kept the image-spec-specific approve_regexp.
* Removed all the groups except for the image-spec group.

[1]: opencontainers/project-template#29

Signed-off-by: W. Trevor King <wking@tremily.us>
wking added a commit to wking/image-spec that referenced this pull request Mar 20, 2017
Pull in changes from [1].  The only changes vs. the upstream version
are:

* Kept the image-spec-specific approve_regexp.
* Removed all the groups except for the image-spec group.

[1]: opencontainers/project-template#29

Signed-off-by: W. Trevor King <wking@tremily.us>
jonboulle added a commit to jonboulle/image-tools that referenced this pull request Apr 6, 2017
This lazily copies image-spec's configuration as updated via
opencontainers/project-template#29 and
opencontainers/image-spec#616

Signed-off-by: Jonathan Boulle <jonathanboulle@gmail.com>
wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Jun 5, 2017
Pull in changes from [1].  The only changes vs. the upstream version
is removing all the groups except for the runtime-spec group.

[1]: opencontainers/project-template#29

Signed-off-by: W. Trevor King <wking@tremily.us>
dattgoswami9lk5g added a commit to dattgoswami9lk5g/bremlinr that referenced this pull request Jun 6, 2022
Pull in changes from [1].  The only changes vs. the upstream version
are:

* Kept the image-spec-specific approve_regexp.
* Removed all the groups except for the image-spec group.

[1]: opencontainers/project-template#29

Signed-off-by: W. Trevor King <wking@tremily.us>
7c00d pushed a commit to 7c00d/J1nHyeockKim that referenced this pull request Jun 6, 2022
Pull in changes from [1].  The only changes vs. the upstream version
are:

* Kept the image-spec-specific approve_regexp.
* Removed all the groups except for the image-spec group.

[1]: opencontainers/project-template#29

Signed-off-by: W. Trevor King <wking@tremily.us>
7c00d added a commit to 7c00d/J1nHyeockKim that referenced this pull request Jun 6, 2022
Pull in changes from [1].  The only changes vs. the upstream version
are:

* Kept the image-spec-specific approve_regexp.
* Removed all the groups except for the image-spec group.

[1]: opencontainers/project-template#29

Signed-off-by: W. Trevor King <wking@tremily.us>
laventuraw added a commit to laventuraw/Kihara-tony0 that referenced this pull request Jun 6, 2022
Pull in changes from [1].  The only changes vs. the upstream version
are:

* Kept the image-spec-specific approve_regexp.
* Removed all the groups except for the image-spec group.

[1]: opencontainers/project-template#29

Signed-off-by: W. Trevor King <wking@tremily.us>
tomalopbsr0tt added a commit to tomalopbsr0tt/fabiojosej that referenced this pull request Oct 6, 2022
Pull in changes from [1].  The only changes vs. the upstream version
are:

* Kept the image-spec-specific approve_regexp.
* Removed all the groups except for the image-spec group.

[1]: opencontainers/project-template#29

Signed-off-by: W. Trevor King <wking@tremily.us>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants