Test-infra: Prevent squashable commits from merging

[tallclair]

It's considered a best practice to respond to PR feedback in new commits, so my PRs often end up with a long list of "Address feedback" commit messages. However, I sometimes come into work in the morning to find that the whole PR merged when it was LGTM'd. I do not think the onus should be on the reviewer to catch this situation.

The simplest solution might be a do-not-merge/needs-squash label (and associated bot command /needs-squash). Ideally the bot would be able to recognize when the PR was squashed and retain the LGTM, but that might be a hard problem to solve, so a simple /squashed might suffice. Another optimization might be to add a keyword to the commit message itself. For instance, pushing a commit that starts with squash: could automatically add the label.

_aside: I'm disappointed that there isn't a squash emoji_

[BenTheElder]

For retaining LGTM on squash see: https://github.com/kubernetes/test-infra/pull/9138
There's a long running effort to enable this, we definitely want that.

I think a /hold\nHolding so I can squash after review comment would be functionally equivalent to the /needs-squash for the moment at least since both would be socially enforced merge blocking rather than automatically enforced.

Lack of squash emoji I don't have a solution for :-( We could at least add one on slack.

[tallclair]

Yeah, using /hold was my first thought. My concern is that over-burdened reviewers might see the hold and immediately dismiss the PR as not needing attention. Also, a squash-specific label would make it more clear what needs to happen for the hold to be removed.

[BenTheElder]

makes sense, WDYT @cjwagner @stevekuznetsov ?

This also reminded me to look at #9138 which looks almost ready to go now, @cjwagner should we poke that one again?

/area prow
/kind enhancement

:jack_o_lantern: = :jack_o_lantern: appears to be the closest thing to a squash :^)

[tallclair]

:jack_o_lantern: = :jack_o_lantern: appears to be the closest thing to a squash :^)

Oh yeah! Good call. I looked up the phylogenetic tree of the :eggplant: to see if it was related to squash (the answer is no).

[stevekuznetsov]

Not clear why we would need that /needs-squash if we have sticky LGTM for squashes? The reviewer could just drop in the /lgtm and leave?

[tallclair]

The /needs-squash is to prevent the PR from automatically merging after /lgtm. Sticky LGTM is orthogonal.

[cjwagner]

This sounds like a good idea to me. This would be easily implemented with the command framework once I get around to finishing that. https://github.com/kubernetes/test-infra/pull/8131

This also reminded me to look at #9138 which looks almost ready to go now, @cjwagner should we poke that one again?

@matthyx will be back next week.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[matthyx]

/remove-lifecycle rotten
might work on it once we have the command framework

[fejta]

How about if we block multiple commits by default, until an org member whitelists the multi PR for a multi commit merge?

[tallclair]

yet another idea: would it be possible for the submit queue to auto-squash some commits as part of the merge? E.g. if a commit message starts with "squash:" then the submit queue would just collapse it into the previous commit prior to merging, without any user interaction (I believe git calls this a "fixup").

[stevekuznetsov]

@tallclair repos can be configured to merge by squashing, and tide already supports squashing using the tide/squash label.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[matthyx]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[matthyx]

/remove-lifecycle stale
@cjwagner any chance you'll finish the command framework?

[cjwagner]

Probably not :disappointed:
I think it was a fun idea, but ultimately not something we really need. There have consistently been more useful things for me to work on.

[matthyx]

Do you need help on it? I could have a look after done with the configtree...

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[matthyx]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[matthyx]

/remove-lifecycle rotten

[matthyx]

@tallclair @stevekuznetsov @fejta I have some time to work on this... so what should be the implementation?

• /needs-squash
• tide performing the squash
• prevent multi-commit PRs from merging

[matthyx]

/assign

[tallclair]

I'm not sure how much more is needed here. We already have tide/squash for auto-squash. We could require approval or at least explicit ACK from the owner before merging something with multiple commits.

[sftim]

:thought_balloon: Maybe reviewers would like to be able to comment on a PR

/lgtm if-squashed

It's different to the original suggestion @tallclair; does it work as an approach do you think?

As an approver for the website, where people often contribute entirely within the GitHub web UI, I'd be happy to add:

/squash
/approve

to website PRs that were labelled lgtm/if-squashed.

I can make this its own issue if people like the idea.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.