New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: prevent merge commits via prow plugin #5376

Open
cblecker opened this Issue Nov 7, 2017 · 15 comments

Comments

Projects
None yet
9 participants
@cblecker
Member

cblecker commented Nov 7, 2017

There's been a number of discussions around merge commits, and why they are bad. One of the more recent ones on kubernetes-dev@ is here.

Right now it's up to the reviewers and approvers to notice that there are merge commits, and withhold lgtm/approve until the author squashes or rebases them away.

kubernetes/kubernetes#51176 was opened to add a verify script to k/k to prevent them there, but I think this could be more easily done in a prow plugin (which would also let other repos easily turn it on).

For example, the GitHub API allows you to list all the commits for a PR, and count the number of "parents" of the commit. If it's >1, then it's a merge commit.

Example:

curl -s -H "Authorization: token ${GITHUB_API_TOKEN}" https://api.github.com/repos/kubernetes/community/pulls/1355/commits | jq '.[] | {sha: .sha, commit_message: .commit.message, number_of_parents: .parents | length}'
{
  "sha": "e4e04b4f25eb767c1b4e8c316c2a2c8ab2e30b3b",
  "commit_message": "sepell error",
  "number_of_parents": 1
}
{
  "sha": "574094e67d7c23beb23edfca014f0fca3e482979",
  "commit_message": "Merge remote-tracking branch 'community/master'",
  "number_of_parents": 2
}
{
  "sha": "964f90233cbb4550bc9ed00f62157b342cd7e58f",
  "commit_message": "spell error",
  "number_of_parents": 1
}
{
  "sha": "3d45caa2c516ecbb6fc7e689620bc35c4017ca72",
  "commit_message": "spell error",
  "number_of_parents": 1
}

If we set this up with a do-not-merge/* class label, then we could prevent auto-merging a PR containing a merge commit. The only issue would be if there is a legitimate case for a merge commit in a PR -- if this was the case, a branch manager or build cop would need to manually merge it as the SQ would refuse.

cc: @kubernetes/sig-contributor-experience-proposals @kubernetes/sig-testing-proposals @lavalamp @sttts @ixdy
/area prow

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Nov 7, 2017

Contributor

Prow is the right place to do this IMO 👍

As side note: we don't fall over merges anymore while publishing, but we loose the commit history before the last merge on a feature branches.

Contributor

sttts commented Nov 7, 2017

Prow is the right place to do this IMO 👍

As side note: we don't fall over merges anymore while publishing, but we loose the commit history before the last merge on a feature branches.

@spxtr spxtr added the ¯\_(ツ)_/¯ label Nov 7, 2017

@fejta-bot

This comment was marked as outdated.

Show comment
Hide comment
@fejta-bot

fejta-bot Feb 7, 2018

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 commented Feb 7, 2018

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

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Feb 7, 2018

Member

/remove-lifecycle stale

Member

cblecker commented Feb 7, 2018

/remove-lifecycle stale

@fejta-bot

This comment was marked as outdated.

Show comment
Hide comment
@fejta-bot

fejta-bot May 8, 2018

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 commented May 8, 2018

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

This comment has been minimized.

Show comment
Hide comment
@fejta

fejta May 8, 2018

Contributor

/remove-lifecycle stale

IMO it would be good to either a) allow merge commits or b) make prow enforce their absence.

Contributor

fejta commented May 8, 2018

/remove-lifecycle stale

IMO it would be good to either a) allow merge commits or b) make prow enforce their absence.

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp May 8, 2018

Member

A very compelling reason to allow merge commits is for feature branches. E.g. kubernetes/kubernetes#63558

Member

lavalamp commented May 8, 2018

A very compelling reason to allow merge commits is for feature branches. E.g. kubernetes/kubernetes#63558

@BenTheElder

This comment has been minimized.

Show comment
Hide comment
@BenTheElder

BenTheElder May 9, 2018

Member

@lavalamp would you actually want the merge commits in the PR to merge it back to master though?
this is about avoiding merge commits in contributor PRs by having the bot notice merge commits in PRs and prompt authors to clean up their history.

Member

BenTheElder commented May 9, 2018

@lavalamp would you actually want the merge commits in the PR to merge it back to master though?
this is about avoiding merge commits in contributor PRs by having the bot notice merge commits in PRs and prompt authors to clean up their history.

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp May 9, 2018

Member

@BenTheElder does the bot know the difference between master and a feature branch? Also, I probably shouldn't lose the history when I re-integrate this back into master.

I agree it's ordinarily a mistake for contributors to have a bunch of merge commits in a PR.

Member

lavalamp commented May 9, 2018

@BenTheElder does the bot know the difference between master and a feature branch? Also, I probably shouldn't lose the history when I re-integrate this back into master.

I agree it's ordinarily a mistake for contributors to have a bunch of merge commits in a PR.

@BenTheElder

This comment has been minimized.

Show comment
Hide comment
@BenTheElder

BenTheElder May 9, 2018

Member

@lavalamp we could always bypass the requirements for snowflake PRs anyhow, but this could definitely be configured by branch.

Member

BenTheElder commented May 9, 2018

@lavalamp we could always bypass the requirements for snowflake PRs anyhow, but this could definitely be configured by branch.

@fejta-bot

This comment was marked as outdated.

Show comment
Hide comment
@fejta-bot

fejta-bot Aug 7, 2018

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 commented Aug 7, 2018

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

@BenTheElder

This comment has been minimized.

Show comment
Hide comment
@BenTheElder

BenTheElder Aug 7, 2018

Member

@cblecker do we still need this? If so we should probably clean it up for help-wanted

Member

BenTheElder commented Aug 7, 2018

@cblecker do we still need this? If so we should probably clean it up for help-wanted

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Aug 7, 2018

Member

Yeah, I think this a good candidate for help wanted.

/remove-lifecycle stale
/help

Member

cblecker commented Aug 7, 2018

Yeah, I think this a good candidate for help wanted.

/remove-lifecycle stale
/help

@alisondy

This comment has been minimized.

Show comment
Hide comment
@alisondy

alisondy Sep 9, 2018

can I pick this up please?

alisondy commented Sep 9, 2018

can I pick this up please?

@BenTheElder

This comment has been minimized.

Show comment
Hide comment
@BenTheElder

BenTheElder Sep 10, 2018

Member

Sure :-)

Member

BenTheElder commented Sep 10, 2018

Sure :-)

@cblecker

This comment has been minimized.

Show comment
Hide comment
@cblecker

cblecker Sep 10, 2018

Member

Please do!

/remove-help

Member

cblecker commented Sep 10, 2018

Please do!

/remove-help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment