Test-infra: Automatically apply the squash merge label on multiple commits

[fejta]

Create a plugin (and/or extend tide) to detect that:

• The repo will not squash commits by default
• Configured a label to request squashing commits
• The author pushed multiple commits to the PR

And then automatically add the squash commits label.

You can configure tide to have a label for requesting a squashed merge:

https://github.com/kubernetes/test-infra/blob/4b5c7c99a851eb427f5c77bd0c8d11526f7b63c4/prow/config/tide.go#L116-L119

When a PR has this label, tide will squash all commits into one before merging the PR so it appears as 1 commit upstream.

It is also possible to configure the default merge strategy for an org or repo:

https://github.com/kubernetes/test-infra/blob/4b5c7c99a851eb427f5c77bd0c8d11526f7b63c4/prow/config/tide.go#L87-L89

Often a PR will amass a bunch of commits like the following:

1) Implement feature X
2) Fix bugs
3) Address comments
4) More feedback
5) Lint errors

Which we want people to squash down into the single "implement feature X" commit before merge. On the other hand, repos in kubernetes org (and many other repos) do not squash by default in order to allow logically distinct commits to remain in the upstream commit history, such as:

1) Decouple X and Y
2) Add feature Z to Y

or:

1) Implement feature
2) Update vendor

The former (lots of "address feedback", "fix misspelled word") type commits) is more common than the latter intentional separation. Especially for PRs from newer contributors.

Therefore I would like to see an extension to either tide or a plugin such that we detect when someone pushes additional commits to a PR and automatically applies the label if the repo is not already configured to squash commits.

The size plugin is probably a good example here:

• Registers to receive push events: https://github.com/kubernetes/test-infra/blob/4b5c7c99a851eb427f5c77bd0c8d11526f7b63c4/prow/plugins/size/size.go#L49
• Adds a label - https://github.com/kubernetes/test-infra/blob/4b5c7c99a851eb427f5c77bd0c8d11526f7b63c4/prow/plugins/size/size.go#L151
• Lists existing labels - https://github.com/kubernetes/test-infra/blob/4b5c7c99a851eb427f5c77bd0c8d11526f7b63c4/prow/plugins/size/size.go#L126

The pull_request object github sends includes a "commits": 3, field https://developer.github.com/v3/pulls/#get-a-single-pull-request
which would need to be added to prow's pullrequest struct: https://github.com/kubernetes/test-infra/blob/c96f7e37bc52f3ed2f3f25b920025e441e78879f/prow/github/types.go#L241

/help

[k8s-ci-robot]

@fejta:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

Create a plugin (and/or extend tide) to detect that:

• The repo will not squash commits by default
• Configured a label to request squashing commits
• The author pushed multiple commits to the PR

And then automatically add the squash commits label.

You can configure tide to have a label for requesting a squashed merge:

https://github.com/kubernetes/test-infra/blob/4b5c7c99a851eb427f5c77bd0c8d11526f7b63c4/prow/config/tide.go#L116-L119

When a PR has this label, tide will squash all commits into one before merging the PR so it appears as 1 commit upstream.

It is also possible to configure the default merge strategy for an org or repo:

https://github.com/kubernetes/test-infra/blob/4b5c7c99a851eb427f5c77bd0c8d11526f7b63c4/prow/config/tide.go#L87-L89

Often a PR will amass a bunch of commits like the following:

1) Implement feature X
2) Fix bugs
3) Address comments
4) More feedback
5) Lint errors

Which we want people to squash down into the single "implement feature X" commit before merge. On the other hand, repos in kubernetes org (and many other repos) do not squash by default in order to allow logically distinct commits to remain in the upstream commit history, such as:

1) Decouple X and Y
2) Add feature Z to Y

or:

1) Implement feature
2) Update vendor

The former (lots of "address feedback", "fix misspelled word") type commits) is more common than the latter intentional separation. Especially for PRs from newer contributors.

Therefore I would like to see an extension to either tide or a plugin such that we detect when someone pushes additional commits to a PR and automatically applies the label if the repo is not already configured to squash commits.

The size plugin is probably a good example here:

• Registers to receive push events: https://github.com/kubernetes/test-infra/blob/4b5c7c99a851eb427f5c77bd0c8d11526f7b63c4/prow/plugins/size/size.go#L49
• Adds a label - https://github.com/kubernetes/test-infra/blob/4b5c7c99a851eb427f5c77bd0c8d11526f7b63c4/prow/plugins/size/size.go#L151
• Lists existing labels - https://github.com/kubernetes/test-infra/blob/4b5c7c99a851eb427f5c77bd0c8d11526f7b63c4/prow/plugins/size/size.go#L126

The pull_request object github sends includes a "commits": 3, field https://developer.github.com/v3/pulls/#get-a-single-pull-request
which would need to be added to prow's pullrequest struct: https://github.com/kubernetes/test-infra/blob/c96f7e37bc52f3ed2f3f25b920025e441e78879f/prow/github/types.go#L241

/help

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.

[adshmh]

/assign

[alvaroaleman]

@fejta why not simply configure squash as default mergemethod on those repos?

It has the exact same semantics as such a plugin. It will squash by default and allows to override that via a /label othermergemethod command.

[chizhg]

@fejta why not simply configure squash as default mergemethod on those repos?

It has the exact same semantics as such a plugin. It will squash by default and allows to override that via a /label othermergemethod command.

+1

We are configuring squash as the default mergemethod for Knative projects as always and it works pretty well, so I'm a bit curious why it's not the case for kubernetes/test-infra repo..

[fejta]

Kubernetes wants to allow multiple commit PRs because this often makes the most sense for large PRs.

However we have too many PRs where people forget to squash and/or add the label. This needs to not be the default behavior.

I'd be in favor of switching to squash by default, but I suspect the community will be surprised to see when prow squashes their commits. So IMO we'd still benefit from this type of plugin.

We should either:

• Default to manual commit management, and tell people to use /label foo if the PR has multiple commits and
• Default to squash, and tell people to /label foo if they want to retain the commits in the PR

[alvaroaleman]

So IMO we'd still benefit from this type of plugin.

Can you please clarify what the difference is between such a plugin and just setting the default mergemethod to squash?

[fejta]

This would involve changing the default mergemethod in tide.

This plugin targets ensuring people know how to configure the merge method by leaving a comment on PRs with multiple commits (telling them how to squash if the default method is not squash and/or merge if the default is squash)

(and yes, that's different from the original idea because you're right that we should just use the right default merge method)