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

Automated way to follow conventional commits when merging PRs with a single commit #1128

Closed
ghost opened this issue Oct 31, 2019 · 8 comments
Labels
domain: ci Anything related to Vector's CI environment needs: requirements Needs a a list of requirements before work can be begin type: enhancement A value-adding code change that enhances its existing functionality.

Comments

@ghost
Copy link

ghost commented Oct 31, 2019

Currently we check that pull request titles follow conventional commits using semantic pull requests. This allows us to ensure that PRs with multiple commits merged using "squash and merge" function in GitHub UI have semantic commit messages.

However, when a PR has just a single commit, the commit message automatically generated by GitHub uses not PR title, but the message from the commit itself. Thus, semantic pull request check does not ensure that the merged commit follows conventional commits. This issue is discussed in zeke/semantic-pull-requests#17.

I'm not sure what it is the best way to have this automated, but I have a proposal. The idea is to disable "squash and merge" in GitHub and instead set up a bot which would merge PRs on behalf of maintainers when asked to do so by one of maintainers in a PR comment. For example, Rust core team does this with bors merging PRs automatically when they are approved by one of maintainers. It might make sense for us to set up something similar.

Related to #1102.

@ghost ghost added type: enhancement A value-adding code change that enhances its existing functionality. domain: operations needs: requirements Needs a a list of requirements before work can be begin labels Oct 31, 2019
@lukesteensen
Copy link
Member

Yeah, I've thought about looking into a bors-like infrastructure for us. It would also help avoid cases where there are weird merge artifacts after "conflicting" PRs like in #1087.

@LucioFranco
Copy link
Contributor

I was always under the assumption the commit that was merged uses the PR title. Was this changed?

@ghost
Copy link
Author

ghost commented Nov 1, 2019

I was always under the assumption the commit that was merged uses the PR title. Was this changed?

I'm not sure was it always like this or not, but ccae97b is an example of merging a single-commit PR without changing the default title.

@LucioFranco
Copy link
Contributor

What confuses me is last night I merged a PR on one of my projects and I changed the commit name.

commit on master
hyperium/tonic@c9b7523

and PR hyperium/tonic#109

You can see that the commit by the user was different? I bring this up because personally I really like this workflow. Bors is good but I dislike merge commits and I don't like to impose commit rules on users. I like squash because as a maintainer you can fix their commits easily. Now if this has changed it won't work but that was my motiviation around using squash and merge.

@ghost
Copy link
Author

ghost commented Nov 1, 2019

@LucioFranco Are you using a browser extension like "Refined GitHub"? It is mentioned in zeke/semantic-pull-requests#17 as changing this behavior.

@LucioFranco
Copy link
Contributor

@a-rodin ah yes I am, so if I understand correctly then refined github is auto populating the field for me, but this still means the final title and message when you click squash does become the commit? So its on the person merging to ensure it follows? That still seems fine to me to enforce, we should all be aware of the convention.

@ghost
Copy link
Author

ghost commented Nov 1, 2019

So its on the person merging to ensure it follows? That still seems fine to me to enforce, we should all be aware of the convention.

Yeah, so basically I was thinking about ways to exclude possibility of a human omission here without additional extension (to make it possible to easily merge from a phone/tablet, for example). But it might not be the highest priority for now.

@binarylogic binarylogic added domain: ci Anything related to Vector's CI environment and removed domain: operations labels Jul 29, 2020
@binarylogic
Copy link
Contributor

Closing since we cannot fix this. Refined Github fixes this and we encourage users to use this if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: ci Anything related to Vector's CI environment needs: requirements Needs a a list of requirements before work can be begin type: enhancement A value-adding code change that enhances its existing functionality.
Projects
None yet
Development

No branches or pull requests

3 participants