search

galaxyproject / galaxy

Data intensive science for everyone.
https://galaxyproject.org
Other
1.42k stars 1.01k forks source link

[24.0] Update title of edited PRs only if base ref changed #19183

Closed nsoranzo closed 6 days ago

nsoranzo commented 1 week ago

Also:

Alternative to https://github.com/galaxyproject/galaxy/pull/19177 .

How to test the changes?

(Select all options that apply)

License

jdavcs commented 1 week ago

So does this fire when the pull request was opened or reopened, or edited AND had its base branch changed?

nsoranzo commented 1 week ago

So does this fire when the pull request was opened or reopened, or edited AND had its base branch changed?

Yes, if you just change the title or description of the PR, the CI job is skipped.

jdavcs commented 1 week ago

So does this fire when the pull request was opened or reopened, or edited AND had its base branch changed?

Yes, if you just change the title or description of the PR, the CI job is skipped.

But then this doesn't really address the concerns discussed in #19177. Yes, we can fix the title by editing just the title as a separate follow-up edit, after it has been renamed incorrectly by the bot - but only if we remember to do so - which we might not (someone might not even know they should do that). I'm sorry, I still think we should not enable automated renaming on edit until the bot handles all those cases correctly.

nsoranzo commented 1 week ago

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.

This would obviously be nice to have, but was never part of the initial implementation, so it can't be held against this bugfix.

What if we want to re-target from one release to another? You'll end up with two prefixes in the title.

This PR is an incremental improvement, we don't need to fix all possible scenarios at once since it's nothing mission-critical. Which was also the reasoning when we merged https://github.com/galaxyproject/galaxy/pull/18835 .

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.

This is fixed by this PR.

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.

This won't happen because this workflow is run only if the target branch matches "release_**" .

jdavcs commented 1 week ago

This would obviously be nice to have, but was never part of the initial implementation, so it can't be held against this bugfix.

The initial implementation introduced the change that makes these problematic cases possible. I think undoing that change is the obvious way to prevent these cases from happening. That said, if you think the benefits of automating the title format outweigh the potential problems that may cause, I won't object.

arash77 commented 1 week ago

I think for now the best thing to do is that on editing if it is necessary the bot just warn by commenting on the PR.

arash77 commented 1 week ago

This is also a problem when we change the base branch from one release to another release, The version will be duplicated.

nsoranzo commented 1 week ago

The initial implementation introduced the change that makes these problematic cases possible. I think undoing that change is the obvious way to prevent these cases from happening. That said, if you think the benefits of automating the title format outweigh the potential problems that may cause, I won't object.

That's indeed exactly my point, the title update on edited helps with a real problem: reviewers may not notice the change in the target branch, which is important because we have different criteria for PRs targeting a release branch. If there is a duplication of prefixes in the PR title, it's just mostly an aesthetic issue that's easily fixable manually. If it actually becomes a recurring issue, I'm sure we can fix it.

I think for now the best thing to do is that on editing if it is necessary the bot just warn by commenting on the PR.

https://xkcd.com/1205/ :wink:

This is also a problem when we change the base branch from one release to another release, The version will be duplicated.

@arash77 I've discussed it above, but if you'd like to work on that, I'd be happy to review it.

jdavcs commented 6 days ago

Changing base to 24.2, right? (it updates #18835 which is 24.2)

arash77 commented 6 days ago

Changing base to 24.2, right? (it updates #18835 which is 24.2)

I think it was 24.0 https://github.com/galaxyproject/galaxy/pull/19019