Skip to content
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

Please don't check signoffs on merge commits #13

Closed
dankohn opened this issue Jul 1, 2017 · 5 comments · Fixed by #14
Closed

Please don't check signoffs on merge commits #13

dankohn opened this issue Jul 1, 2017 · 5 comments · Fixed by #14
Labels

Comments

@dankohn
Copy link

dankohn commented Jul 1, 2017

Hi, https://github.com/coreinfrastructure/best-practices-badge is a happy user of your DCObot. The issue is that there is no --signoff option in git for merge commits, which is a standard part of our workflow with feature branches. Here is a workflow where we currently get stuck:

(master)$ git checkout -b feature-branch
# make some changes
(feature-branch)$ git commit -sam 'Adding features'
# Changes have occurred on master so need to add them for easier merge
(feature-branch)$ git fetch
(feature-branch)$ git merge origin/master
# Save default commit message
(feature-branch)$ git push
# This now fails the DCObot check because the merge commit is not signed.

This alternative workflow works, but is obviously tedious:

# First 3 steps are the same
(feature-branch)$ git merge origin/master
# Save default commit message
(feature-branch)$ git commit --amend -s
# Commit message now has signoff line
(feature-branch)$ git push
# This now passes the DCObot check.

Or, I could manually add the Signoff line to the proposed git merge commit message, which would allow me to skip the --amend step.

Could you please add an option to the DCObot (and probably make it the default) not to check merge commits. I'm separately requesting that git add a -s option to git merge. Thanks.

Cc @david-a-wheeler @mkdolan @caniszczyk

@dankohn dankohn changed the title Please don't check signatures on merge commits Please don't check signoffs on merge commits Jul 1, 2017
@dankohn
Copy link
Author

dankohn commented Jul 1, 2017

Git mailing list reference for the git merge --signoff feature request: https://public-inbox.org/git/CAHv71zK5SqbwrBFX=a8-DY9H3KT4FEyMgv__p2gZzNr0WUAPUw@mail.gmail.com/T/#u

@bkeepers
Copy link
Contributor

bkeepers commented Jul 3, 2017

@dankohn thanks for the report. I pushed a change to ignore merge commits for now. Any open PRs will need to be pushed again for the checks to pass.

I'm not opposed to adding a feature in the future to allow people to turn this back on, but it's not currently a priority for me.

@dankohn
Copy link
Author

dankohn commented Jul 3, 2017

Thanks very much. I'm also having a colleague create a patch for git to allow --signoff for merge commits. https://public-inbox.org/git/xmqqwp7pl7yh.fsf@gitster.mtv.corp.google.com/T/#t

Separately, I wanted to comment on your blog post: http://opensoul.org/2016/07/03/cii-best-practices/. Note that we already fetch as much info as possible if you use GitHub as your repo, to try to reduce the manual question answering. We call it the BadgeApp, for short, but would be open to alternative names.

We would also greatly appreciate any assistance GitHub could provide in publicizing the project. For, example, we would love a mention on pages like https://opensource.guide/best-practices/ or https://help.github.com/articles/helping-people-contribute-to-your-project/ that projects should get a best practices badge. I'd be happy to discuss further at dan at linuxfoundation.org if you might be interested.

@github-actions
Copy link

🎉 This issue has been resolved in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@david-a-wheeler
Copy link

Thank you so much!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants