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

Do not overwrite PR title on edit #19177

Closed
wants to merge 2 commits into from
Closed

Conversation

jdavcs
Copy link
Member

@jdavcs jdavcs commented Nov 21, 2024

The new title bot will overwrite the PR title even if you edit that title manually. I think that's not what we want it to do: there may be legitimate cases when we remove the version prefix from the title, and we don't want the bot to immediately add it back.

This change ensures the title is auto-updated only once, when the PR is opened.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@jdavcs jdavcs requested review from arash77 and a team November 21, 2024 14:26
@github-actions github-actions bot added this to the 25.0 milestone Nov 21, 2024
@nsoranzo
Copy link
Member

I suppose that's about #19096 ? Why would you like to remove the "[24.2]"?

I suggested adding edited because often a pull is re-targeted to a different branch, at which point you want the bot to update the title as well.

@jdavcs
Copy link
Member Author

jdavcs commented Nov 21, 2024

I suppose that's about #19096 ? Why would you like to remove the "[24.2]"?

Very minor reason, really. Just an old PR that was a follow-up to another that was part of the release, so I didn't think of it as a regular bug and so didn't want to have it marked as something we add after the fact.

I suggested adding edited because often a pull is re-targeted to a different branch, at which point you want the bot to update the title as well.

I think we need the option to manually override it. I can think of 3 reasons:

  1. It only works one way. So when you re-target from dev to release_24.2, you'll get the renaming, but when you re-target the opposite way (which is not an impossible scenario), it won't remove the prefix. If you have to add it manually in the first case, you'll remember to remove it in the second, otherwise you're likely to forget.
  2. What if we want to re-target from one release to another? You'll end up with two prefixes in the title.
  3. If I manually edit the title, that implies that I know what I'm doing - I don't expect my edit to be undone. It's too much magic.

@arash77
Copy link
Collaborator

arash77 commented Nov 21, 2024

I agree with the scenarios you mentioned, but I think they could get automated.

@jdavcs
Copy link
Member Author

jdavcs commented Nov 21, 2024

Automating such scenarios would be great. However, that would need to account for a variety of cases, not just the ones I mentioned. Here's just one more example: let's say we have a work branch "foo1.2": all PRs would be renamed as "[1.2] old pr title" - again, not what we expect.

So, automating this would be nice. But until we have that automation implemented (covering all such cases), I think we should not override the title when the PR is manually edited.

@arash77
Copy link
Collaborator

arash77 commented Nov 21, 2024

You're right.
Also, for edited PRs, the bot could simply suggest a new title in a comment instead.

@jdavcs
Copy link
Member Author

jdavcs commented Nov 21, 2024

You're right. Also, for edited PRs, the bot could simply suggest a new title in a comment instead.

I like the comment idea! (on edit only; on open the bot should rename it, of course)

Co-authored-by: Nicola Soranzo <nicola.soranzo@gmail.com>
@jdavcs
Copy link
Member Author

jdavcs commented Nov 22, 2024

Closed in favor of #19183

@jdavcs jdavcs closed this Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants