Handle existing upgrade PRs #267

alextu · 2024-11-26T09:01:44Z

Fix #252

Do not execute the update if a branch with the same version already exists
Do not create the PR if a closed one for the same version already exists
PRs can be recreated by setting options.recreateClosedPullRequests to true.

Fix #211

Close existing open PR targeting older versions

alextu · 2024-11-26T09:26:37Z

@StefMa please take a look as well, if you can try this on your side that would be great as well 🙂

README.md

src/main/java/org/gradle/wrapperupgrade/PullRequestUtils.java

README.md

erichaagdev

Requesting changes because ignoreClosedPullRequests should be renamed to recreateClosedPullRequests.

build.gradle

StefMa · 2024-11-27T15:33:04Z

src/main/java/org/gradle/wrapperupgrade/UpgradeWrapper.java

+            return;
+        }
+        PullRequestUtils utils = new PullRequestUtils(pullRequests(params));
+        if (utils.openPrExists(params.prBranch)) {


I'm not sure if I understand correctly, but will this branch ever reached? 🤔

We have the check above branchExist.
If it exist -> return
Here we are asking for openPrExists (for this branch name?)
If so, return

However, if that branchExist (because there is an openPr), then we return already in Line 71?! 🤔
No?

It other words:
If there NO branch, then there is NO PR.
But if a branch exist (and maybe a PR), this will never be reached because we return early in Line 74.

Thanks for the feedback, you're right: there should not be a case where a branch does not exist and an opened PR exists with the same branch, I forgot that GitHub closes the PR automatically in that case. I will fix this in a new PR

@StefMa please check #269

StefMa · 2024-11-27T15:38:54Z

src/main/java/org/gradle/wrapperupgrade/UpgradeWrapper.java

+import org.kohsuke.github.GHRepository;
 import org.kohsuke.github.GitHub;
 import org.kohsuke.github.GitHubBuilder;


I guess it makes finally sense to extract the GitHub API communcation behind an interface so we can test it easiliy 😇

Yeah, tbh I'd rather have better e2e testing, we'll see what we do next.

StefMa

Havent tested it yet, will do probably tomorrow mroning (UTC+1) 😅
One question about an if branch that might not be reached.
Also a suggeston to move the Github API calls to an interface to abstract it away.
Right now it feels a bit messy to use it directly "everywhere"...
Also it makes testing a bit hard.

Otherwise looks good to me 🙃
Thanks for the addtitions.
Cant wait to have them 🚀

Oh.. and sorry for the review after merge 🙈
I didn't find the time until now..

StefMa · 2024-11-28T09:09:54Z

Tested:
Downgrade Gradle to 8.11. Pushed that to a branch named gradle-8.11.
Set baseBranch to gradle-8.11.

First run:
Open PR (StefMa#1).
Run it, output is:

GitHub branch 'wrapperbot/wrapper-upgrade-gradle-plugin/gradle-wrapper-8.11.1' to upgrade Gradle Wrapper to 8.11.1 already exists for project 'wrapper-upgrade-gradle-plugin'

Expected ✅

Second run:
Closed the PR not deleted the branch.
Run it, output is:

GitHub branch 'wrapperbot/wrapper-upgrade-gradle-plugin/gradle-wrapper-8.11.1' to upgrade Gradle Wrapper to 8.11.1 already exists for project 'wrapper-upgrade-gradle-plugin'

I guess the output should be

A closed pull request from branch 'wrapperbot/wrapper-upgrade-gradle-plugin/gradle-wrapper-8.11.1' to upgrade Gradle Wrapper to 8.11.1 already exists for project 'wrapper-upgrade-gradle-plugin'. Use `recreateClosedPullRequests` option to recreate it.

Third run:
Deleted the branch
Run it, output is:

A closed pull request from branch 'wrapperbot/wrapper-upgrade-gradle-plugin/gradle-wrapper-8.11.1' to upgrade Gradle Wrapper to 8.11.1 already exists for project 'wrapper-upgrade-gradle-plugin'. Use `recreateClosedPullRequests` option to recreate it.

I guess the output should be

PR created

Because its not there?! 🤷 🤔

Fourth run:
Branch is still deleted, added recreateClosedPullRequests.
Run it, output is:

https://github.com/StefMa/wrapper-upgrade-gradle-plugin/pull/2

Expected ✅

StefMa · 2024-11-28T09:10:51Z

README.md

+| `options.gitCommitExtraArgs`         | List of additional git commit arguments                                                                                                                                   |
+| `options.allowPreRelease`            | Boolean: `true` will get the latest Maven/Gradle version even if it's a pre-release. Default is `false`.                                                                  |
+| `options.labels`                     | Optional list of label (names) that will be added to the pull request.                                                                                                    |
+| `options.recreateClosedPullRequests` | Boolean: `true` will recreate the pull request if a closed pull request with the same branch name exists. `false` will not recreate the pull request. Default is `false`. |


I just stumbled over this while testing it.
Maybe we should remove the last s.
Its just a single PR, right?
options.recreateClosedPullRequest

Good point, will rename it

alextu · 2024-11-28T09:22:46Z

Second run: Closed the PR not deleted the branch. Run it, output is:

GitHub branch 'wrapperbot/wrapper-upgrade-gradle-plugin/gradle-wrapper-8.11.1' to upgrade Gradle Wrapper to 8.11.1 already exists for project 'wrapper-upgrade-gradle-plugin'

I guess the output should be

A closed pull request from branch 'wrapperbot/wrapper-upgrade-gradle-plugin/gradle-wrapper-8.11.1' to upgrade Gradle Wrapper to 8.11.1 already exists for project 'wrapper-upgrade-gradle-plugin'. Use `recreateClosedPullRequests` option to recreate it.

That message would be misleading as the branch still exists and we should not do anything if the branch exists. I would stick with the current simple approach and message: existing branch = don't go further.

Third run: Deleted the branch Run it, output is:

A closed pull request from branch 'wrapperbot/wrapper-upgrade-gradle-plugin/gradle-wrapper-8.11.1' to upgrade Gradle Wrapper to 8.11.1 already exists for project 'wrapper-upgrade-gradle-plugin'. Use `recreateClosedPullRequests` option to recreate it.

I guess the output should be

PR created

Because its not there?! 🤷 🤔

Not sure to understand the suggestion 🤔. As I see it: a PR exists and is closed, so we notified the user and no new PR is created.

alextu added 4 commits November 25, 2024 18:44

Do not create PR if a closed PR exists by default (#252)

4a6fa77

Close PRs that target older build tool versions (#211)

5e3c6e2

Add docs (#211)

e070eb5

Rephrase message (#252)

4d069aa

alextu requested review from guylabs, welandaz and erichaagdev November 26, 2024 09:01

Rename ignoreExistingClosedPr to ignoreClosedPRs

324f80f

guylabs requested changes Nov 26, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

Rename ignoreExistingClosedPr to ignoreClosedPullRequests

c48ad4c

erichaagdev reviewed Nov 26, 2024

View reviewed changes

src/main/java/org/gradle/wrapperupgrade/PullRequestUtils.java Outdated Show resolved Hide resolved

guylabs approved these changes Nov 26, 2024

View reviewed changes

erichaagdev reviewed Nov 26, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

erichaagdev suggested changes Nov 26, 2024

View reviewed changes

alextu added 2 commits November 27, 2024 10:50

Rename ignoreClosedPullRequests to recreateClosedPullRequests

2326cde

Check branch existence and have better info messages

a72121d

alextu requested a review from erichaagdev November 27, 2024 13:39

erichaagdev approved these changes Nov 27, 2024

View reviewed changes

build.gradle Show resolved Hide resolved

alextu merged commit 48b4fc4 into main Nov 27, 2024
13 checks passed

alextu deleted the atual/check-branch branch November 27, 2024 15:11

StefMa reviewed Nov 27, 2024

View reviewed changes

alextu mentioned this pull request Nov 28, 2024

Remove unnecessary open pr check #269

Open

StefMa reviewed Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle existing upgrade PRs #267

Handle existing upgrade PRs #267

alextu commented Nov 26, 2024 •

edited

Loading

alextu commented Nov 26, 2024

erichaagdev left a comment

StefMa Nov 27, 2024 •

edited

Loading

alextu Nov 27, 2024

alextu Nov 28, 2024

StefMa Nov 27, 2024

alextu Nov 28, 2024

StefMa left a comment •

edited

Loading

StefMa commented Nov 28, 2024

StefMa Nov 28, 2024

alextu Nov 28, 2024

alextu commented Nov 28, 2024 •

edited

Loading

Handle existing upgrade PRs #267

Handle existing upgrade PRs #267

Conversation

alextu commented Nov 26, 2024 • edited Loading

alextu commented Nov 26, 2024

erichaagdev left a comment

Choose a reason for hiding this comment

StefMa Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

alextu Nov 27, 2024

Choose a reason for hiding this comment

alextu Nov 28, 2024

Choose a reason for hiding this comment

StefMa Nov 27, 2024

Choose a reason for hiding this comment

alextu Nov 28, 2024

Choose a reason for hiding this comment

StefMa left a comment • edited Loading

Choose a reason for hiding this comment

StefMa commented Nov 28, 2024

StefMa Nov 28, 2024

Choose a reason for hiding this comment

alextu Nov 28, 2024

Choose a reason for hiding this comment

alextu commented Nov 28, 2024 • edited Loading

alextu commented Nov 26, 2024 •

edited

Loading

StefMa Nov 27, 2024 •

edited

Loading

StefMa left a comment •

edited

Loading

alextu commented Nov 28, 2024 •

edited

Loading