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

Commit messages should be checked as part of the Pull Request #851

Open
AmiStrn opened this issue Jun 16, 2021 · 5 comments
Open

Commit messages should be checked as part of the Pull Request #851

AmiStrn opened this issue Jun 16, 2021 · 5 comments
Labels
documentation Improvements or additions to documentation enhancement Enhancement or improvement to existing feature or request

Comments

@AmiStrn
Copy link
Contributor

AmiStrn commented Jun 16, 2021

Is your feature request related to a problem? Please describe.
Many commits to main have been committed without any message or with a messy unreadable message. Prior to the fork, Elasticsearch's commit messages are normally helpful for understanding the reasons for a given commit. Recently I noticed a severe degradation in this respect, in the OpenSearch repo. For example, see commits #829, #712, #818 (there are many more across the project, in opensearch-dashboards and various plugin repositories as well).

Describe the solution you'd like
In CONTRIBUTING.md add the following:

  • Guidelines for writing good commit messages
  • Requirement that the commit message is provided by the pull request author as part of the review process.

Additional context
Discussion in OpenSearch forum - https://discuss.opendistrocommunity.dev/t/missing-commit-messages-and-changelogs/

@AmiStrn AmiStrn added the enhancement Enhancement or improvement to existing feature or request label Jun 16, 2021
@AmiStrn AmiStrn mentioned this issue Jun 16, 2021
1 task
@dblock
Copy link
Member

dblock commented Jun 16, 2021

We've been iterating on org-wide docs. I think I want to thin out the ones in this repo and start pointing to the ones in https://github.com/opensearch-project/.github like in #853. Let's see what @CEHENKLE says.

@sandeshkr419
Copy link
Contributor

sandeshkr419 commented Apr 4, 2024

I propose the following to maintain a good code commit history hygiene:

  1. We disable “Allow merge commits” in the (https://github.blog/changelog/2022-08-23-new-options-for-controlling-the-default-commit-message-when-merging-a-pull-request/) - I’m assuming individual commits will not matter for 90%+ cases. Only keep “Allow squash merging” with default to pull request title & description which can be editable.

  2. Add a ‘code merge guideline’ (please feel free to suggest alternate title) in maintainer responsibility to have meaningful title and description before merging, or within this section: https://github.com/opensearch-project/.github/blob/main/RESPONSIBILITIES.md#maintain-overall-health-of-the-repo something similar to what security plugin does.

  3. We can add the below section on top in PR creation template and add a checkbox for “PR commit summary provided”. This can help a code maintainer draft the final commit message.

## Proposed commit message
<!-- Summarize your PR here, this will be the basis for creating the final squashed commit message -->
[Brief summary of changes.]

To uphold this, it may require some vigilantism (maintainers/contributors giving feedback to other maintainers) for some time, but we should be progressing in right direction.
I recommend we enforce the repo setting for all the repos in OpenSearch Project.

@andrross
Copy link
Member

andrross commented Apr 4, 2024

@sandeshkr419

  1. Merge commits are already disabled, at least in this repo. Changing the default commit message to "pull request title & description" would be the change here, as currently it is set to "Default Message"
  2. This seems reasonable.
  3. If we use this suggestion along with "pull request title & description" then the default commit message would contain the "## Proposed commit message" heading text along with the checklist.

For me, personally, I try my best to write a good commit message (pretty much following everything described in this blog: https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html) and then force push my PRs to contain a single commit so that the default squash will take my commit and message verbatim. I don't love the idea of using the PR description in the commit message as I see them as two different things: The PR is a proposal. It's editable and more conversational (e.g. "I don't love the way I solved x here, looking for feedback on better ideas"), whereas the actual commit and its message is what I'm proposing to commit to the permanent and immutable record.

@sandeshkr419
Copy link
Contributor

@andrross

Maintainers responsibility should be added right away I guess irrespective of what we decide with PR templates and repo settings: opensearch-project/.github#194

Also, I think that the idea of 1/ Proposed commit message in PR template + 2/ changing the default commit message to pull request title & description together makes sense. Can add the comment in PR template that it can be drafted after the code changes are finalized - of-course this is editable by maintainers.

This way, less chances of the commit trail being pushed as commit messages, but, but, but, we would still need some maintainers (can't enforce this, it can only be voluntarily) to provide feedback on any violations to uphold the good commit hygiene.

@andrross
Copy link
Member

andrross commented Apr 6, 2024

Also, I think that the idea of 1/ Proposed commit message in PR template + 2/ changing the default commit message to pull request title & description together makes sense.

@sandeshkr419 I guess the thing I don't love about this is that every PR merge will require hand-editing the commit message, because the maintainer doing the merge will have to delete everything from the PR description that wasn't in the "Proposed commit message" section. With the current configuration, assuming the contributor force pushes to their PR branch to keep the PR to a single commit and writes a good commit message, then the maintainer doesn't have to do anything except click merge.

ritty27 pushed a commit to ritty27/OpenSearch that referenced this issue May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement Enhancement or improvement to existing feature or request
Projects
None yet
Development

No branches or pull requests

5 participants