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

Leave follow-up comments for committers when best practices not followed #14

Closed
brettcannon opened this Issue May 23, 2017 · 22 comments

Comments

Projects
None yet
8 participants
@brettcannon
Member

brettcannon commented May 23, 2017

Since we don't have pre-commit hooks to block people from doing something that goes against what we outline in the devguide, we could leave a comment post-commit to remind core devs of how they should be doing something. That way there's at least a feedback loop to help core devs change their habits going forward.

@brettcannon

This comment has been minimized.

Member

brettcannon commented Jun 3, 2017

Comments could involve:

  1. Forgetting the version number, e.g. [3.6]
  2. Forgetting to namespace PR numbers, e.g. (GH-1234) instead of (#1234)
  3. Forgetting to drop the bullet points in the default commit message when multiple commits used to construct the PR
@terryjreedy

This comment has been minimized.

Member

terryjreedy commented Aug 15, 2017

The first two only involve manual backports. #3 is really needed.

A couple of times, I have edited multiple * points to just a few, with coherent sentences. Would that be flagged? This would be okay if I know since the * is not really needed.

@brettcannon

This comment has been minimized.

Member

brettcannon commented Aug 15, 2017

@terryjreedy it's not designed yet so no one knows. 😄

@Mariatta

This comment has been minimized.

Member

Mariatta commented Sep 1, 2017

Replacing PR number from #1234 to GH-1234 should be done in the master PR too.

Mariatta added a commit that referenced this issue Jan 26, 2018

Mariatta added a commit that referenced this issue Jan 26, 2018

Remind core dev to replace # into GH- in the commit message. (GH-82)
To address number 2 of #14

If a PR was merged and the commit message was not changed from `#NNNN` to `GH-NNNN`, bedevere-bot will leave a comment to remind core dev to update it next time.
For example:
@Mariatta: Please replace # with GH- in the commit message next time. Thanks!
@Mariatta

This comment has been minimized.

Member

Mariatta commented Jan 26, 2018

Number 2 (Forgetting to namespace PR numbers, e.g. (GH-1234) instead of (#1234)) is now done :)

@brettcannon

This comment has been minimized.

Member

brettcannon commented Jan 28, 2018

And Miss Islington takes care of number 1, so that just leaves number 3!

@gpshead

This comment has been minimized.

Member

gpshead commented Jan 30, 2018

Why can't bedevere just rename the pull request titles itself? I'm seeing lots of

Please replace # with GH- in the commit message next time. Thanks!

messages after a PR is merged. Where did the # come from in the first place? Why should a human need to fix this manually?

@Mariatta

This comment has been minimized.

Member

Mariatta commented Jan 30, 2018

@gpshead, that part can't currently be automated. Or rather we haven't found an agreeable solution on how to automate it.

The # comes into play after you pressed "Squash and merge" button, and before you pressed the "Confirm squash and merge"
See screenshot: (from python/cpython#5435)

screen shot 2018-01-29 at 6 56 47 pm

Inside the textbox, we need to change (#5435) into (GH-5435) manually.

@tiran

This comment has been minimized.

Member

tiran commented Jan 30, 2018

Ah, I got it :) Thanks Mariatta!

@berkerpeksag

This comment has been minimized.

Member

berkerpeksag commented Jun 22, 2018

Adding another item to Brett's list:

  1. Forgetting to drop the second PR number for backport PRs, e.g. bpo-42: Fix spam_eggs() (GH-66) instead of bpo-42: Fix spam_eggs() (GH-66) (GH-67) This is already documented at https://devguide.python.org/gitbootcamp/#backporting-merged-changes (Note that @miss-islington automatically removes it when merging a backport PR, but the PR needs to be approved by a core developer in order to do that.)
@merwok

This comment has been minimized.

merwok commented Jun 22, 2018

The pre-filled commit message is annoying enough that I edit it 99% of the time (for work or my rare merges for CPython), but I could see myself forgetting about the two other things half the time.

An idea: have one of the bots post a message before the merge with the fixed title (commit first line) and a reminder about the commit body.

A radical idea: let core reviewers approve PRs and bots merge them.
https://graydon.livejournal.com/186550.html
https://julien.danjou.info/pastamaker/

@Mariatta

This comment has been minimized.

Member

Mariatta commented Jun 22, 2018

An idea: have one of the bots post a message before the merge with the fixed title (commit first line) and a reminder about the commit body.

If people don't mind the noise, we can have the bot post a reminder message once a core dev approved the PR, or once it has awaiting merge label.

A radical idea: let core reviewers approve PRs and bots merge them.

I personally also just prefer having a bot does the merge, and take care of all these administrative things (and we don't have to keep discussing this) :)

Related issue about automerge by bot python/core-workflow#29, and also in several mailing lists threads (but now I can't find them).

The challenge is how to provide the commit message to the bot.

My suggestion is as follows:

  • for a PR that we want the bot to automerge, first edit the PR title and body with the correct commit message
  • apply "automerge" label
  • bot will merge upon seeing the automerge label, taking the current PR title and body as commit message
  • bot removes "automerge" label when it's done
  • What if we forgot to edit the PR title and body before applying the label?
@merwok

This comment has been minimized.

merwok commented Jun 22, 2018

Why edit the PR title? The bot can replace # with GH and such.

Agreed that a way to give the commit message is needed.

@Mariatta

This comment has been minimized.

Member

Mariatta commented Jun 22, 2018

Why edit the PR title?

To make the PR title and body as the proper commit message for the bot.
The PR title provided by contributor is not always appropriate commit message.

@merwok

This comment has been minimized.

merwok commented Jun 22, 2018

Ah right! I think I assumed that the core reviewer will already edit the PR title if needed, and the bot would only have to change [3.6] and GH-123.

Having a special message containing both title and body seems to be the simplest, most robust thing.

@terryjreedy

This comment has been minimized.

Member

terryjreedy commented Jun 22, 2018

I often edit PR titles, including my own. Edits to the PR title are currently ignored by github when merging. I have to scroll up, copy, scroll down, and paste. A minor but real annoyance, especially when the change is small and hard to notice.

@zware

This comment has been minimized.

Member

zware commented Jun 22, 2018

I'd suggest having the merge-bot (which I think we should add :)) add a new comment with the suggested commit message (derived from PR title and commit message(s), but explicitly without the things we complain about :)), and the core dev can edit it at will and add a trigger word at the end (@mergebot go or similar) to signal to the bot that it's ready to go with the message now in the bot's comment. Optionally, it could update the PR title to match so that only one place has to be edited to keep things straight.

@Mariatta

This comment has been minimized.

Member

Mariatta commented Jun 22, 2018

PR title are currently ignored by github when merging

Right, and so I suggest we have our own bot that will look at the PR title and body, and use that as commit message :)

I think I've brought up in a mailing list in the past, of issuing the command@python-bot merge {commit message}, but someone commented that it will be more hassle to type out those things when they can press one button.
So now I'm suggesting a label automerge instead. But issuing command works for me too.
(Just create saved reply for shortcut)

@terryjreedy

This comment has been minimized.

Member

terryjreedy commented Jun 22, 2018

When I have edited title and message and have decided I am ready to merge if and when CI passes, I would like to be able to click [automerge] and move on to something else. I presume a command is possible if anyone actually prefers that and say so.

@brettcannon

This comment has been minimized.

Member

brettcannon commented Jun 22, 2018

There seem to be two things to choose:

  1. Where to pull the commit message from (i.e. title + first message or a custom message in another comment)
  2. How to trigger the bot (i.e. label or magical comment)

My vote is for:

  1. Title + first message (since that's what is going to be in the metadata and what people see and search for later)
  2. Label (because we all make typos sometimes 😉 )

And to be honest, anyone who doesn't like using this can simply not use it as it isn't like the Green Button won't still be there as an escape hatch.

And to answer Mariatta's question of "What if we forgot to edit the PR title and body before applying the label?", it's the same result as if someone forgets to edit the commit message now. Otherwise we will need a three-way handshake of label -> message reminding you to edit -> something to say "I'm absolutely sure", and that simply sounds like too much work.

Mariatta added a commit to Mariatta/miss-islington that referenced this issue Sep 10, 2018

Automerge PR labeled with "automerge".
Merge the PR if:
- PR has "awaiting merge" and "automerge" labels, and
- all CI status checks have passed, and
- replace # with GH-
- get the commit message from PR title and description

Closes python/bedevere#14

@Mariatta Mariatta referenced this issue Sep 10, 2018

Merged

Automerge PR labeled with "automerge". #146

1 of 1 task complete
@Mariatta

This comment has been minimized.

Member

Mariatta commented Sep 10, 2018

I've made the PR to let miss-islington merge the PR: python/miss-islington#146

The conditions to let miss-islington merge the PR:

  • ALL CI status checks passed
  • it has awaiting merge label
  • it has automerge label (new label that we should create)

It will take the PR title and PR description as the commit message.

It will replace # with GH-.

@Mariatta

This comment has been minimized.

Member

Mariatta commented Sep 11, 2018

If nobody has any objections I will deploy it tomorrow morning. ☝️

Mariatta added a commit to Mariatta/miss-islington that referenced this issue Sep 11, 2018

Automerge PR labeled with "automerge".
Merge the PR if:
- PR has "awaiting merge" and "automerge" labels, and
- all CI status checks have passed, and
- replace # with GH-
- get the commit message from PR title and description

Closes python/bedevere#14

Mariatta added a commit to Mariatta/miss-islington that referenced this issue Sep 11, 2018

Automerge PR labeled with "automerge".
Merge the PR if:
- PR has "awaiting merge" and "automerge" labels, and
- all CI status checks have passed, and
- replace # with GH-
- get the commit message from PR title and description

Closes python/bedevere#14

Mariatta added a commit to python/miss-islington that referenced this issue Sep 11, 2018

Automerge PR labeled with "automerge". (#146)
Merge the PR if:
- PR has "awaiting merge" and "automerge" labels, and
- all CI status checks have passed.

It will:
- replace `#` with `GH-`
- get the commit message from PR title and description

Closes python/bedevere#14
Closes python/core-workflow#29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment