-
Notifications
You must be signed in to change notification settings - Fork 969
Fix used and unused dependencies #4303
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
Fix used and unused dependencies #4303
Conversation
| <artifactId>aws-json-protocol</artifactId> | ||
| <version>${awsjavasdk.version}</version> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding it to every service module, can we add it to this pom? https://github.com/aws/aws-sdk-java-v2/blob/master/services/pom.xml This way, if we need to change anything, say artifact ID, we can do it just in one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a note to the description about it
Given that codecatalyst does not use http-auth-aws this dependency was added to all the services but that one instead of just adding it to the services/pom.xml parent pom.
So, we can add it to the services/pom.xml and live with one warning of unused dependency for codecatalyst, or we can just add it to all of the services but codecatalyst as done here and avoid having any warnings, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, should've read the description first 😅 Agreed we should add the dependency to each service module since not all services use them. The last thing we want is adding unused dependency since it'll increase JAR size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update this template though https://github.com/aws/aws-sdk-java-v2/blob/master/services/new-service-template/pom.xml This is used to create new service modules. We need to figure out how to add this dependency conditionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template is already updated, we do not have at the moment a way to "opt-out" other than editing the resulting pom. I can take a look at that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline, new services will be addressed in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added on #4309
|
SonarCloud Quality Gate failed.
|











Motivation and Context
Added or removed dependencies to clear all the warning related to use but undeclared, or unused but declared dependencies.
Given that codecatalyst does not use
http-auth-awsthis dependency was added to all the services but that one instead of just adding it to theservices/pom.xmlparent pom.Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License