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

Move dependency management and plugin management to parent pom #1260

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

siri-varma
Copy link

Description

  • I think having all the versions defined in the parent pom is a good idea.
  • Moved the versions for testcontainer-module and spring-boot-example to parent pom. Depending on the feedback from this PR will work on rest of the modules in later prs.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #940

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

Sorry, something went wrong.

@siri-varma siri-varma requested review from a team as code owners March 14, 2025 21:13
@siri-varma
Copy link
Author

@salaboy / @cicoyle Can you please approve the builds to run ? Want to make sure all builds pass

Signed-off-by: sirivarma <[email protected]>
Signed-off-by: sirivarma <[email protected]>
Signed-off-by: sirivarma <[email protected]>
Signed-off-by: sirivarma <[email protected]>
Signed-off-by: sirivarma <[email protected]>
@siri-varma siri-varma force-pushed the users/svegiraju/refactor-pom-1 branch from c897dec to d2ecf18 Compare March 14, 2025 22:58
Signed-off-by: sirivarma <[email protected]>
artur-ciocanu
artur-ciocanu previously approved these changes Mar 16, 2025
Copy link
Contributor

@artur-ciocanu artur-ciocanu left a comment

Choose a reason for hiding this comment

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

@siri-varma thank you so much for this effort. I have added a few comments. Mainly I want to ensure that if you went through the trouble of extracting some of version properties you should make sure that all dependency versions are extracted.

Also we should double check that the right scopes are used, so we don't accidentally pack dependencies that should not be deployed to Maven Central.

Signed-off-by: Siri Varma Vegiraju <[email protected]>
@siri-varma
Copy link
Author

@cicoyle Would you be able to approve the build on this PR please ?

artur-ciocanu
artur-ciocanu previously approved these changes Mar 17, 2025
Copy link
Contributor

@artur-ciocanu artur-ciocanu left a comment

Choose a reason for hiding this comment

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

Approving for CI

@artur-ciocanu
Copy link
Contributor

@siri-varma could you please verify this failing CI runs: https://github.com/dapr/java-sdk/actions/runs/13902606663/job/38899059339?pr=1260.

It seems that there are some issue with Dapr IT tests that can be found in sdk-tests folder.

svegiraju-microsoft and others added 6 commits March 17, 2025 11:03
Signed-off-by: Siri Varma Vegiraju <[email protected]>
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: Siri Varma Vegiraju <[email protected]>
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
…factor-pom-1
Signed-off-by: siri-varma <[email protected]>
Signed-off-by: siri-varma <[email protected]>
@siri-varma
Copy link
Author

siri-varma commented Mar 17, 2025

@artur-ciocanu thanks for taking the time to review. There were two sets of error

  1. Due to updating spring boot from 3.2.6 to 3.4
    A) Reverted back the change to 3.2.6 because we still have to support 3.3.x versions
  2. Coverage ratio for sdk-tests
    A) Fixed this one by overriding the coverage ratio for tests to be 0%

Will need the approval again for the build to run. Thank you

artur-ciocanu
artur-ciocanu previously approved these changes Mar 17, 2025
Copy link
Contributor

@artur-ciocanu artur-ciocanu left a comment

Choose a reason for hiding this comment

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

Re-approving for CI

Signed-off-by: sirivarma <[email protected]>
artur-ciocanu
artur-ciocanu previously approved these changes Mar 18, 2025
Copy link
Contributor

@artur-ciocanu artur-ciocanu left a comment

Choose a reason for hiding this comment

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

Reapproving for CI

@siri-varma
Copy link
Author

@artur-ciocanu Finally ! These changes are good to go. Thanks a ton for reapproving the CI build multiple times.

@artur-ciocanu
Copy link
Contributor

@cicoyle could you please take a look and approve if things look good. Thank you!

@siri-varma
Copy link
Author

/assign

@@ -33,6 +33,7 @@
<logback-core.version>1.5.16</logback-core.version>
<wiremock.version>3.9.1</wiremock.version>
<testcontainers-test.version>1.20.0</testcontainers-test.version>
<jacoco-maven-plugin.coverage-ratio>0%</jacoco-maven-plugin.coverage-ratio>
Copy link
Contributor

Choose a reason for hiding this comment

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

@siri-varma I am a bit lost.. why is this 0% ?

Copy link
Author

Choose a reason for hiding this comment

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

The JaCoCo plugin was expecting coverage for this module, but since it's a test module and code coverage typically doesn't apply to tests, I set the coverage to 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment noting this for future reference?

Copy link
Author

Choose a reason for hiding this comment

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

Added comment

Copy link
Contributor

@salaboy salaboy left a comment

Choose a reason for hiding this comment

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

@siri-varma I've added a few comments.. apologies for the delay.

Signed-off-by: sirivarma <[email protected]>
…iri-varma/java-sdk into users/svegiraju/refactor-pom-1
@siri-varma
Copy link
Author

@siri-varma I've added a few comments.. apologies for the delay.

@salaboy addressed all the comments

artur-ciocanu
artur-ciocanu previously approved these changes Mar 20, 2025
Copy link
Contributor

@artur-ciocanu artur-ciocanu left a comment

Choose a reason for hiding this comment

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

Approve for CI

@siri-varma
Copy link
Author

siri-varma commented Mar 20, 2025

Thank you for approving Artur. I think the build failed because of the flaky integration test

@artur-ciocanu artur-ciocanu requested a review from salaboy March 20, 2025 21:40
Copy link
Contributor

@cicoyle cicoyle left a comment

Choose a reason for hiding this comment

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

One comment if you can add a comment in code around the 0% test coverage area for future reference

Signed-off-by: sirivarma <[email protected]>
Signed-off-by: sirivarma <[email protected]>
…iri-varma/java-sdk into users/svegiraju/refactor-pom-1
@siri-varma
Copy link
Author

One comment if you can add a comment in code around the 0% test coverage area for future reference

@cicoyle done. Added the comment

@artur-ciocanu
Copy link
Contributor

artur-ciocanu commented Mar 21, 2025

@salaboy the build is 🟢 could you please take another look and see if everything looks good, so we could merge this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor maven POMs to put all information about the dependencies/plugins in the root POM
5 participants