Poll: stop changing `#` to `GH-` on PR merge

We have a workflow step of changing ‘#’ to ‘GH-’ when we merge pull requests on GitHub. Originally this was implemented to namespace GitHub issue numbers so that if/when we migrate to another tool the commit messages are unambiguous as to what issue on what platform they are linked to.

As I still from time to time forget to change ‘#’ to ‘GH-’ and I see that other committers forget that too, I’m wondering if we just stop doing this. Hence this poll!

  • Keep the current workflow: change ‘#123’ to ‘GH-123’ on every PR merge
  • Simplify the merge workflow and keep commit messages as GitHub suggests

0 voters

Python was first hosted on SourceForge. Hopefully, we managed to keep the same bug identifiers when we moved to our own Roundup instance. But the GitHub migration is likely to loose bug numbers since we already used the first ~20k identifiers with pull requests, and GitHub shares the same identifier space for issues and pull requests. In practice, bpo-xxx and GH-xxx are likely to mean two different things forever. It’s painful to have to modify a PR manually, but I would prefer to continue to do that.

Can’t we fix (configure) GitHub instead, since GitHub is the one who adds " (#xxx)" to the commit title!

3 Likes

GH-### will make it much easier to work with the history later, if we switch away from GH. (And there is a good chance I’ll still be going through history 10 years from now). But if you forget to add GH- here and there, it’s not the end of the world.

I don’t see any disadvantage of the bot adding GH-, in cases you decide to use the bot.

2 Likes

I don’t think that’s easy or even possible.

To be clear I think the bots can continue do their thing. My poll is about not nagging people with slightly annoying comments after merge and slightly simplify the merge workflow for them. It’s not a deal breaker kind of thing though. We can still recommend to change to GH-, just not as a loosely enforced requirement.

1 Like

Now that GitHub Actions is more capable than when we added the bots message, maybe we can do a post-commit fix up?

Otherwise, we can plan to do a rewrite of all # numbers when we migrate to the next service? Might be nice to write that script now. (Though staying on git will preserve commit ids and the rewrite would break those, but perhaps that wouldn’t matter?)

There is already an existing working solution: use automerge. I don’t use automerge for the reasons listed in my post RFE automerge enhancements

Also, I’m not sure what happens if I put an automerge label while the CI is running, and then the author push further changes. Does it remove the label? Or does it push the code that I didn’t review?

We already lost the mapping from Subversion revision to Git sha1. Hopefully, the service mapping Mercurial sha1 to Git sha1 still works. But until when? I would prefer to not change Git sha1 tomorrow.

It would be nice if a post-merge bot could modify a commit to replace #xxx with GH-xxx, but only if there is a way to ensure that it is before anyone can fetch this commit.

This is brought up and discussed in Potential security issue with "🤖 automerge" · Issue #325 · python/core-workflow · GitHub

There are a couple solution proposed and we need decision on what should happen.

It’s currently not possible but GitHub is aware of our ask and the motivation behind our ask.

3 Likes

If GitHub can’t be configured to use (GH-NNNN) instead of (#NNNN), what about the possibility of a pre-commit hook that rejects commits if GH- isn’t used?

I think people would learn more quickly that way, and the format would be more useful going forward because we can be sure it’s being used consistently.

1 Like

It’s the merge commit through the UI that adds it. I’m not sure we can apply a pre-commit hook to a server side commit. Hence the post-commit reminder.

1 Like

We can’t. If we can’t get a setting for this from GitHub then the only way to avoid this is to get everyone using the auto-merge bot. That means people helping out to improve it so that everyone is happy to use it.

1 Like

I think we have more important things to get from them (like working CI filters, and less noisy notifications, and anything else we’ll need to do issues over there).

Presumably to get everyone using the bot we’d have to provide a way to customise the commit message still, yes? I basically always modify the entire commit message to make sure it’s usefully searchable later on, and doesn’t include a list of pointless sub-commit messages, and also to remove people’s handles because it leads to even more annoying notifications. I believe the bot doesn’t do that yet.

But once we’re happy enough, we just disable merging entirely and make the bot an admin, yeah? Or are we going to end up needing the bot that reminds people to start reminding people to not merge manually?

Edit the PR title and body, these will be used as the commit message.

There’s an open PR to address this. Use GH to signify Github handle of the user instead of "@". by maxking · Pull Request #275 · python/miss-islington · GitHub

The bot doesn’t need to be an admin. We just need to add branch protection rule, similar to how we protect 3.5 and 3.6 branches to only release managers. Example:

1 Like

I’ve created a PR to address this. (Re-request core review when there is a new commit to the approved PR by Mariatta · Pull Request #220 · python/bedevere · GitHub)
If there is new commit to an already approved PR, it will move back to the awaiting core review stage.

I’ve grown to really not like this method of doing this :-/. Requiring that the first message be changed means that any responses to the original content of that message start looking like non-sequiturs unless you dig through the edits to the messages. (This can be fixed currently by reverting the first message back to its original form after merge, but that’s horrifyingly bad UX :))

What if we have miss-islington add her own message to the PR containing the proposed commit message (defaulting to the title and body, as now) which the core dev can edit at any time before merge to get the message they want?

I’m still partial to supporting a trigger command in a message and providing the commit message there, but that idea has already been shot down a couple of times :slight_smile:

2 Likes

Any interest in submitting the commit message via a web form, similar to blurb-it?

1 Like

I would like that better than the current method, yes :slight_smile:

Hopefully it can be as convenient as the current web form…

Updating the title isn’t much of a burden really, so jumping out to another site and doing an auth dance is going to raise the bar significantly.

We’re probably better to just accept that occasionally we’ll forget to update the title, and next time we migrate VCS we can fix them all up.