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

Boot 3 upgrade doesn't add XML Bind dependency #485

Merged
merged 17 commits into from
Jun 20, 2024

Conversation

BoykoAlex
Copy link
Contributor

Unit test demonstrating an issue upgrading Petclinic to Boot 3.
jakarta.xml.bind-api dependency should be added explicitly but it's not because rewrite-maven dependency resolution thinks that this dependency is present on the compile scope already for some reason via transitive dependencies.

timtebeek and others added 2 commits May 23, 2024 00:36
…ot2/Boot3UpgradeTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek timtebeek marked this pull request as draft May 22, 2024 22:36
…ot2/Boot3UpgradeTest.java

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@timtebeek
Copy link
Contributor

Locally I've found the test to still fail with the following, but we'll get a second opinion from CI just to be sure

diff --git a/project/pom.xml b/project/pom.xml
index 4369491..f80ac0a 100644
--- a/project/pom.xml
+++ b/project/pom.xml
@@ -30,10 +30,6 @@ 
       <artifactId>jakarta.validation-api</artifactId>
     </dependency>
     <dependency>
-      <groupId>jakarta.xml.bind</groupId>
-      <artifactId>jakarta.xml.bind-api</artifactId>
-    </dependency>
-    <dependency>
       <groupId>org.springframework.boot</groupId>
       <artifactId>spring-boot-starter-data-jpa</artifactId>
     </dependency>

@timtebeek
Copy link
Contributor

Ok CI passes; that's an improvement! I'll see about polishing this up for addition to your tests then, and figure out what's troubling locally. :)

@timtebeek timtebeek marked this pull request as ready for review June 20, 2024 12:46
@timtebeek timtebeek self-requested a review June 20, 2024 12:46
Copy link
Contributor

@timtebeek timtebeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this resolved, thanks a lot! Must help you as well to see this work on a common example case, finally.

@timtebeek
Copy link
Contributor

If I understood this comment correctly we should remove the expected added dependency here again

I'll go ahead and try that here.

@timtebeek timtebeek merged commit 24af0a4 into openrewrite:main Jun 20, 2024
2 checks passed
@timtebeek
Copy link
Contributor

Thanks again @BoykoAlex ! Really great not to have those unnecessary dependencies added, and good to have a comprehensive test to guard that going forward.

@BoykoAlex
Copy link
Contributor Author

@timtebeek Thanks a lot for merging all those PRs so quickly!!!

@timtebeek
Copy link
Contributor

Of course! Really appreciated how you dove deep to get very targeted fixes with great effects downstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
4 participants