update pullapprove config #404
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
On Thu, Jun 29, 2017 at 05:33:32AM +0000, Ma Shimiao wrote:
update to v2
See also the similar opencontainers/project-template#29. Differences
between your current 84e4cb5 and that template:
* You have the always_pending block as a root entry [1], while I put
it under group_defaults [2]. More on my positioning choice in
opencontainers/project-template@a5e7e37.
* You drop the ^ anchors for LGTM and Rejected, but those are useful.
For example, it allows comments like “@mrunalp Needs your re-LGTM”
[3] which shouldn't count as votes.
* You don't have a ‘conditions’ block [4]. More on that in
opencontainers/project-template@dfd50da.
[1]: 84e4cb5#diff-8c1463eff758cd513e6643bde738656dR7
[2]: https://github.com/opencontainers/project-template/pull/29/files#diff-8c1463eff758cd513e6643bde738656dR17
[3]: #254 (comment)
[4]: https://github.com/opencontainers/project-template/pull/29/files#diff-8c1463eff758cd513e6643bde738656dR20
|
|
On Thu, Jul 06, 2017 at 02:38:10AM +0000, 梁辰晔 (Liang Chenye) wrote:
Test comment: LGTM
PullApprove uses the current master .pullapprove.yml to check a PR,
not the version suggested by the PR (and using the current rules to
approve new rules makes more sense than using new rules to approve
themselves ;). So any testing will need to happen after this PR
lands, or in a separate (dummy?) repository.
|
|
On 07/11/2017 03:37 AM, W. Trevor King wrote:
* You drop the ^ anchors for LGTM and Rejected, but those are useful.
For example, it allows comments like ***@***.*** Needs your re-LGTM”
[3] which shouldn't count as votes.
That' a problem. But there is no good way to solve some maintainers
like to comment as 'xxxxxx LGTM'.
And in the case "@mrunalp Needs your re-LGTM",
I think there are three situations:
a. author of this comment is not a maintainer, then this will not count
b. author of this comment is a maintainer, he should reviewed and approved.
'LGTM' will not count twice.
c. author of this comment is a maintainer, he reviewed but didn't apparoved.
when he ask others to re-LGTM, he must also thinks this is LGTM.
* You don't have a ‘conditions’ block [4]. More on that in
***@***.***
I think this should also work for other branches, so there is no need to add 'conditions'
|
|
On Mon, Jul 10, 2017 at 06:41:00PM -0700, Ma Shimiao wrote:
On 07/11/2017 03:37 AM, W. Trevor King wrote:
> * You drop the ^ anchors for LGTM and Rejected, but those are useful.
> For example, it allows comments like ***@***.*** Needs your re-LGTM”
> [3] which shouldn't count as votes.
That' a problem. But there is no good way to solve some maintainers
like to comment as 'xxxxxx LGTM'.
Yes there is, you just tell them that doesn't count and ask them to
comment again with a leading LGTM. After a few rounds of that, folks
should remember. Alternatively, folks can use GitHub's
review-approval, in which case the wording doesn't matter.
And in the case ***@***.*** Needs your re-LGTM",
I think there are three situations:
a. author of this comment is not a maintainer, then this will not count
b. author of this comment is a maintainer, he should reviewed and approved.
'LGTM' will not count twice.
c. author of this comment is a maintainer, he reviewed but didn't apparoved.
when he ask others to re-LGTM, he must also thinks this is LGTM.
Those seem like the most likely cases. But another example is “Sorry
guys, not LGTM” [1], which falls into none of those cases.
I think it's better to be strict and risk false-negatives than it is
to be lax and risk false-positives for approval. I'm more open to
unanchored Rejected, since inaction is fairly safe and recoverable.
But if something accidentally slips into master because of an
accidental LGTM, that's more annoying to recover from (you have to
file an explicit reversion, and then collect LGTMs on that).
> * You don't have a ‘conditions’ block [4]. More on that in
> ***@***.***
I think this should also work for other branches, so there is no
need to add 'conditions'
There are a few cases here:
a. A branch used for release development, for example master or our
abandoned v1.0.0.rc1 [2] (which we should probably delete).
b. A branch used for feature development, with authors collaborating
on an OCI-repo branch instead of in their private repository
(e.g. if you'd wanted to file a PR against #56 while it was in
flight). In this repo, #56 is the only PR so far based on an
OCI-repo branch.
c. A branch used for build artifacts, e.g. if we gained a gh-pages
branch. But there's probably a way to avoid PullApprove checks for
branches like that (when the pusher has some secret token? Or when
we just trust folks with push access to not push to the
auto-generated branch?).
If the goal is to only ever have case (a), then yeah, we don't need a
conditions block. I'm ok with that, and either way we can revisit it
if/when we get a long-term branch besides master that we expect to
maintain.
I think there's also a benefit to consistency between OCI
repositories, so if you feel like we don't want a conditions block,
could you file a project-template PR reverting
opencontainers/project-template@dfd50da
with your motivation?
[1]: opencontainers/image-spec#221 (comment)
[2]: https://github.com/opencontainers/runtime-tools/tree/v1.0.0.rc1
[3]: https://github.com/opencontainers/image-spec/tree/add-governance-docs
|
Mashimiao
force-pushed
the
update-pullapprove-config
branch
from
84e4cb5 to
ab7c945
Compare
|
just make changes to update to v2, OK? |
|
On Tue, Jul 11, 2017 at 04:58:14AM +0000, Ma Shimiao wrote:
just make changes to update to v2, OK?
With 84e4cb5 → ab7c945, you add a ‘conditions’ block and restore the ^
anchors.
You still differ from project-template's version in the position of
always_pending [1], you single-quote the regexps while
project-template does not, and you allow lowercase ‘wip’ or uppercase
‘WIP’ anywhere in the title to mark work-in-progress while
project-template requires a leading ‘WIP’:
$ diff -u ../project-template/.pullapprove.yml <(git cat-file -p ab7c945:.pullapprove.yml)
--- ../project-template/.pullapprove.yml 2017-03-30 09:10:44.868833782 -0700
+++ /dev/fd/63 2017-07-10 23:08:10.438849839 -0700
@@ -4,42 +4,25 @@
signed_off_by:
required: true
+always_pending:
+ title_regex: '(WIP|wip)'
+ explanation: 'Work in progress...'
+
group_defaults:
required: 2
approve_by_comment:
enabled: true
- approve_regex: ^LGTM
- reject_regex: ^Rejected
+ approve_regex: '^LGTM'
+ reject_regex: '^Rejected'
reset_on_push:
enabled: true
author_approval:
ignored: true
- always_pending:
- title_regex: ^WIP
- explanation: 'Work in progress...'
conditions:
branches:
- master
groups:
…
The quoting and always_pending positions are no-op changes, so I'm
fine with your versions (I'd rather match the project-template version
in both cases for pan-OCI consistency, but whatever ;).
The unanchored, somewhat-case-insensitive title_regexp seems
unnecessarily lax to me, since I expect anyone using that feature to
use a prefix WIP (which is all project-template allows). There's also
a chance that someone will use “foo: wipe bar” or some such as a
title, which would match the unanchored ‘wip’ case in your current
regex.
[1]: #404 (comment)
|
update to v2 Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
Mashimiao
force-pushed
the
update-pullapprove-config
branch
from
ab7c945 to
967bbd1
Compare
|
On 07/11/2017 02:16 PM, W. Trevor King wrote:
since I expect anyone using that feature to
use a prefix WIP (which is all project-template allows). There's also
a chance that someone will use “foo: wipe bar” or some such as a
title, which would match the unanchored ‘wip’ case in your current
regex.
Fine, updated
|
|
LGTM |
|
The current tip only differs from project-template with the no-op
regex quoting and always_pending position changes (and the list of
groups, but we want that ;):
$ diff -u ../project-template/.pullapprove.yml <(git cat-file -p 967bbd1:.pullapprove.yml)
--- ../project-template/.pullapprove.yml 2017-03-30 09:10:44.868833782 -0700
+++ /dev/fd/63 2017-07-12 09:36:01.031028929 -0700
@@ -4,42 +4,25 @@
signed_off_by:
required: true
+always_pending:
+ title_regex: '^WIP'
+ explanation: 'Work in progress...'
+
group_defaults:
required: 2
approve_by_comment:
enabled: true
- approve_regex: ^LGTM
- reject_regex: ^Rejected
+ approve_regex: '^LGTM'
+ reject_regex: '^Rejected'
reset_on_push:
enabled: true
author_approval:
ignored: true
- always_pending:
- title_regex: ^WIP
- explanation: 'Work in progress...'
conditions:
branches:
- master
groups:
…
I don't see a point to such no-op divergence, but pan-OCI consistency
aside, I'm fine with 967bbd1.
|
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
update to v2
Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com