Test-infra: prow: trigger submit process manually

[awly]

Proposal

Separate lgtm+approval from submitting and add new /submit command to trigger merge of a PR.

Why

PRs may require multiple rounds of reviews/changes.

Fixup commits are a straightforward way to address comments and convenient for reviewers to look at. But final merged PR needs those commits squashed.

As a PR author, i want to address comments in a series of small commits, get lgtm+approval, squash the commits and merge. But this would race with the submit queue.

So now it takes an extra review round-trip for reviewers to say "LGTM but squash fixup commits", then for me to squash and push and say "PTAL" and finally get /lgtm.

I'd like to hear of others feel the same way and would prefer to kick off /submit manually.

[awly]

cc @immutablet

[BenTheElder]

we've discussed that in the past, currently one way you would do this is /lgtm\n/hold and then the author does /hold cancel

[BenTheElder]

/area prow
/cc @cjwagner

[immutableT]

Have we considered adding a check for un-squashed commits?
Scenario, reviewer is happy with the change (lgtm/approve), but the PR still needs to be squashed. Once the submitter squashes the PR, merge is triggered. This save a trip for the reviewer back to the PR.

Better yet, could we just squash it before merge.

[BenTheElder]

@immutableT we have discussed it but have not implemented, since when you squash you then force push and without comparing the code to an old copy or something we don't know that you didn't force push new code. ...Which means we also need to store the old version / hash somewhere. It's a good idea, but not completely trivial and hasn't been in terribly high demand.

[fejta]

Have we considered adding a check for un-squashed commits?

Yes, see https://github.com/kubernetes/test-infra/issues/5376 although note that we explicitly want to allow multiple commit PRs sometimes.

i want to address comments in a series of small commits, get lgtm+approval, squash the commits and merge

The lack of a /submit command -- aka applying the do-not-merge/hold label by default -- doesn't seem like the issue here.

It seems more like a request to make the lgtm label sticky when certain types of PR updates are made, and/or configure the tide submit-queue to always/sometimes squash and merge instead of just merge.

Right now the lgtm plugin removes the lgtm label whenever you update your PR (to add a commit, rebase, squash existing commits, etc). And we've made an international decision (on the main k/k repo) to not use github's squash-and-merge functionality by default.

Thoughts on duping this to an existing discussions?

• https://github.com/kubernetes/test-infra/issues/7899
• https://github.com/kubernetes/test-infra/issues/1755
• https://github.com/kubernetes/contrib/issues/1853

[awly]

Hmm, I think https://github.com/kubernetes/contrib/issues/1853 would be ideal (looks like it got auto-closed).
Alternatively, detecting manual squash (compare PR diff before/after push) to keep lgtm label sticky would work too.

Can https://github.com/kubernetes/contrib/issues/1853 be re-opened to dup into?

[BenTheElder]

@awly that one's a bit stale since we're not using the submit-queue anymore going forward.

All repos but kubernetes/kubernetes are already on tide which does support specific repos using squash merges (though it's for all PRs, not per-pr, see merge_method in the readme).

[cjwagner]

We should make the 'remove lgtm label on PR update' feature optional by repo to support this workflow in repos where it is preferred (this one!).

The actual /submit command could be implemented pretty easily. It will be trivial to implement it after https://github.com/kubernetes/test-infra/pull/8131 is in.

[cblecker]

The concern from me would be the author doing self-review. If it's a straight squash, we could try to detect that (diff LOC to ensure it's the same, even though it's a different commit hash).. but there isn't a one of us who hasn't screwed up a rebase at least once. Having someone else to re-review helps with that.

So the squashing use case we could potentially deal with safely.. but I think the "remove LGTM on PR sync" is important for all repos to have on.

[kargakis]

Alternatively, detecting manual squash (compare PR diff before/after push) to keep lgtm label sticky would work too.

Issue for this is https://github.com/kubernetes/test-infra/issues/6353

[awly]

@BenTheElder do you know if kubernetes/kubernetes will use squash merges after switching to tide?

@kargakis ah, great! This and squash merges address all of my concerns. I really hope kubernetes/kubernetes gets either one.

[BenTheElder]

Unlikely. Many PRs have good logical commit sequencing and squashing
commits strips signed commits.

I think kubernetes will prefer to keep git history over auto squashing, but
for small repos, PRs generally have one logical commit worth of changes
anyhow.

On Fri, Jun 1, 2018, 9:40 AM Andrew Lytvynov [e-mail]
wrote:

@BenTheElder https://github.com/BenTheElder do you know if
kubernetes/kubernetes will use squash merges after switching to tide?

@kargakis https://github.com/kargakis ah, great! This and squash merges
address all of my concerns. I really hope kubernetes/kubernetes gets either
one.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/8167#issuecomment-393938678,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA4BqyFmjQbNAdgpodd7k5vufrTxXkJmks5t4W6TgaJpZM4UUHDE
.