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

Better commit messages #6

Open
SaschaMann opened this issue Jun 2, 2021 · 3 comments
Open

Better commit messages #6

SaschaMann opened this issue Jun 2, 2021 · 3 comments
Labels
help wanted Extra attention is needed

Comments

@SaschaMann
Copy link
Contributor

SaschaMann commented Jun 2, 2021

Currently the commit messages of the bot commits aren't very descriptive. They have a generic name and a reference to a commit that may or may not be the commit that triggered the build. There are a few ways these could be improved. Two cases need to be considered:

  1. Builds triggered from within this repo, e.g. by pushing to main or through manual triggers.
  2. Builds triggered by the tracks, e.g. from changes to the .appends dir.

The first case is fairly simple to handle:

  • on push: Use the commit message of the commit that triggers it.
  • on workflow_dispatch: Add an input for the commit/PR title and message.

Perhaps with a standard addendum that points to the doc page describing org-wide-files and that it will be automerged.

The second case is more complicated. We could use the commit message of the commit that changed .appends. Either by making it part of the repository_dispatch payload, or by adding the commit hash to the payload and fetching the message from the GitHub API. However, since we're now dealing with untrusted data, we might have to sanitise it or add proper safeguards. Currently the repository dispatch doesn't contain any info other than the track that triggered the build, which is not changeable by users.


Personally, I don't particularly care for the commit history of files that are changed upstream and I don't expect there to be all that many changes in general, so I don't think this is a priority for now. However, I know some maintainers might disagree :D


cc @ee7

@SaschaMann SaschaMann added the help wanted Extra attention is needed label Jun 2, 2021
@ee7
Copy link
Member

ee7 commented May 13, 2022

However, I know some maintainers might disagree :D

I'm happy to see that I'm not the only one who edits the default commit title of

🤖 Sync org-wide files to upstream repo

See:

(Go Peter!)

I like the first much more than the second:

But I don't mind this being a low priority or non-priority. Just as long as a PR in configlet isn't auto-merged...

The new "set the default commit title to be that of the PR body" setting would also help me with e.g. exercism/nim-test-runner#138, where I can't merge before maintainers-admin approval.

@SaschaMann
Copy link
Contributor Author

But I don't mind this being a low priority or non-priority. Just as long as a PR in configlet isn't auto-merged...

Automerging would likely be a requirement for better commit messages. Otherwise there's no guarantee that a PR might not contain several changes at once and our current strategy of not running builds for minor changes (e.g. typos) to not annoy maintainers wouldn't work out anymore.

@ErikSchierboom
Copy link
Member

Auto-merging would be really great, but I have a script that I can run locally that can help with this. That said, improving the commit message would be really nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants