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

Change aws-sdk-java to an optional dependency #4797

Merged
merged 1 commit into from
Dec 21, 2023

Conversation

dagnir
Copy link
Contributor

@dagnir dagnir commented Dec 20, 2023

Having it as a normal dependency causes third dependencies like SLF4J to also be included, unshaded in the JAR.

Motivation and Context

Modifications

Testing

Build the bundle JAR using the publishing profile:

mvn clean install -P publishing  -Dspotbugs.skip -DskipTests -Dcheckstyle.skip -Djapicmp.skip -Dgpg.skip=true

Open the generated JAR and verify that there are no unshaded dependencies apart from org.reactivestreams which I believe is by design (this is also true for older versions of the Bundle).

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

Having it as a normal dependency causes third dependencies like SLF4J to also
be included, unshaded in the JAR.
@dagnir dagnir requested a review from a team as a code owner December 20, 2023 23:22
@joviegas
Copy link
Contributor

Open the generated JAR and verify that there are no unshaded dependencies apart from org.reactivestreams which I believe is by design (this is also true for older versions of the Bundle).

Could you please elaborate on where is this design exist ?

Are there any cases of backward incompatibility issues ?, as in when there is version upgraded can we have class not found runtime issues ?

Why dont we exclude the third dependencies like SLF4J explicitly ?

@dagnir
Copy link
Contributor Author

dagnir commented Dec 21, 2023

Could you please elaborate on where is this design exist ?

It's only a hunch, but we can fix this later if needed since org.reactivestreams was never shaded in the bundle. I think the intention is because if we shade the org.reactivestreams classes, they won't be interoperable with external libraries that use them like RxJava

Why dont we exclude the third dependencies like SLF4J explicitly ?

not sure what you mean by this. We can exclude the classes because they're used for logging. They're shaded so that other libraries don't use them.

@joviegas
Copy link
Contributor

Could you please elaborate on where is this design exist ?

It's only a hunch, but we can fix this later if needed since org.reactivestreams was never shaded in the bundle. I think the intention is because if we shade the org.reactivestreams classes, they won't be interoperable with external libraries that use them like RxJava

Why dont we exclude the third dependencies like SLF4J explicitly ?

not sure what you mean by this. We can exclude the classes because they're used for logging. They're shaded so that other libraries don't use them.

I meant something like this

    <dependencies>
        <dependency>
            <groupId>software.amazon.awssdk</groupId>
            <artifactId>aws-sdk-java</artifactId>
            <version>${awsjavasdk.version}</version>
            <exclusions>
                <exclusion>
                    <groupId>org.slf4j</groupId>
                    <artifactId>slf4j-api</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
    </dependencies>

@dagnir
Copy link
Contributor Author

dagnir commented Dec 21, 2023

See #4778 for context. We don't want to exclude SLF4J entirely, we just want to make sure only the shaded classes are in the JAR.

Copy link

sonarcloud bot commented Dec 21, 2023

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@dagnir dagnir merged commit a88908b into master Dec 21, 2023
14 checks passed
@shwethags
Copy link

Hi, which SDK release version contains this fix?

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

Successfully merging this pull request may close these issues.

3 participants