Add PullApprove checks

[caniszczyk]

Taking advantage of PullApprove will allow us to enforce the rules of 2 LGTMs from the maintainers before anything is merged.

https://pullapprove.com/opencontainers/runtime-spec/

[caniszczyk]

cc: @crosbymichael

[crosbymichael]

Do you think we should do the reset on push ?

[caniszczyk]

@crosbymichael I agree, I think reset on push is a good idea

[caniszczyk]

LGTM (no idea why the build is failing @crosbymichael though)

[caniszczyk]

LGTM (minus the build failure)

[crosbymichael]

@caniszczyk

I think it maybe because your commit description and subject line are not separated by a new line.

Add PullApprove checks
https://pullapprove.com/opencontainers/runtime-spec/

Signed-off-by: Chris Aniszczyk <[email protected]>

VS

Add PullApprove checks

https://pullapprove.com/opencontainers/runtime-spec/

Signed-off-by: Chris Aniszczyk <[email protected]>

[caniszczyk]

@crosbymichael should be fixed now, otherwise I'll install Vincent's git-validation thing locally and see what's wrong

[crosbymichael]

@caniszczyk i think it is worse now just looking at your commit ;)

[vbatts]

Do we have a strict 2 LGTM rule?

On Thu, May 26, 2016, 16:55 Michael Crosby [email protected] wrote:

@caniszczyk https://github.com/caniszczyk i think it is worse now just
looking at your commit ;)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#457 (comment)

[caniszczyk]

@crosbymichael what the hell, it's absolutely separated by a new line

@vbatts can comment on the strict 2 LGTM rule in the maintainers guide

[crosbymichael]

@vbatts we have always had that. 1 LGTM does not give time for others to look at and review. Its basically showing respect to your other maintainers that you wait for their reviews.

[crosbymichael]

Add PullApprove checks
Add PullApprove support:
https://pullapprove.com/opencontainers/runtime-spec/

Signed-off-by: Chris Aniszczyk <[email protected]>

worse ;)

[wking]

On Thu, May 26, 2016 at 02:06:20PM -0700, Chris Aniszczyk wrote:

@crosbymichael what the hell, it's absolutely separated by a new line

You need two newlines (subject/body should be separated by one blank
line).

[wking]

On Thu, May 26, 2016 at 02:07:24PM -0700, Michael Crosby wrote:

@vbatts we have always had that. 1 LGTM does not give time for
others to look at and review. Its basically showing respect to your
other maintainers that you wait for their reviews.

And for maintainer-authored PRs, I expect the self-LGTM will still
count as one vote (I seem to recall @vbatts and @philips talking about
that at some point, but can't dig up a reference).

[crosbymichael]

@wking no it does not count as one. You cannot LGTM your own PRs, its up to others to review or else what is the point.

[wking]

On Thu, May 26, 2016 at 02:32:30PM -0700, Michael Crosby wrote:

@wking no it does not count as one. You cannot LGTM your own PRs,
its up to others to review or else what is the point.

Increased development speed. Everything's in version control, so
folks can always file “I don't think this was a good idea issues/PRs”.

[caniszczyk]

@wking I just must be having some weird line ending issues on OSX as I don't see the problem locally or via https://patch-diff.githubusercontent.com/raw/opencontainers/runtime-spec/pull/457.patch

[crosbymichael]

@caniszczyk ahh, i think the extra test that is failing is because you pushed the branch to this repo and not your fork.

I opened #458 and its good being from my fork with your commit

[caniszczyk]

See #458

[cyphar]

@wking @crosbymichael

Self-LGTMs should not count as part of the 2 LGTM rule (although, this was never made clear in CONTRIBUTING.md -- I'll make a PR to fix this). IMO it's disrespectful to other maintainers to act that way. As an aside, I don't think the PullApprove tool actually checks for this. I would make a PR to fix it, but it's proprietary.