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

Shaded and non shaded versions #428

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

Conversation

danieldbower
Copy link

@danieldbower danieldbower commented Aug 3, 2023

Corrects the url of the sdk documentation as the given url does not work.

Minor fix added to specify the version number for the maven-source-plugin

I also bumped the version to ensure I had some maven settings correct.

I was not able to run the redis module tests as that seems to be an integration test requiring some infrastructure.

The pom will now create a shaded version java-client-with-deps and a non-shaded version at java-client.

In this version, documentation referencing the shaded version's maven coordinates would have to change from this:

            <dependency>
                <groupId>io.split.client</groupId>
                <artifactId>java-client</artifactId>
                <version>4.9.0</version>
            </dependency>

to this

            <dependency>
                <groupId>io.split.client</groupId>
                <artifactId>java-client-with-deps</artifactId>
                <version>4.9.0</version>
            </dependency>

The original maven coordinates would be for the non-shaded version.

Why do this?
Simpler management of dependencies with tools like scanning for vulnerabilities and updates.

…r with-deps and a non-shaded version. The pom that is produced just has the dependencies for the shaded version though. Need to figure out how to supply the non-shaded deps
@danieldbower danieldbower changed the title Ddb shaded and non shaded versions Shaded and non shaded versions Aug 3, 2023
@danieldbower
Copy link
Author

While this works, I'm increasingly convinced that the correct way to do it is a separate module so that the dependency list in the pom is correct in both instances.

@danieldbower
Copy link
Author

Reasoning for the separate module: https://www.mail-archive.com/[email protected]/msg139854.html

@danieldbower danieldbower marked this pull request as ready for review August 3, 2023 16:34
@danieldbower danieldbower requested a review from a team as a code owner August 3, 2023 16:34
@jbreton
Copy link

jbreton commented Nov 7, 2023

I'm using this package in a kotlin project. While it's working fine using JVM, it fails when using the native build (Graalvm). We believe the native build is not able to load some of the shaded dependencies.
It's quite hard to prove since SplitClientImp says the sdk is ready but all calls to getTreatment are failling. There are no other warnings or errors. It seems to at least call the API since we're getting a 400 Bad request error when using an invalid API Key.

@danieldbower
Copy link
Author

Are there any suggestions or concerns with this PR that would increase its chances of being merged?

@danieldbower
Copy link
Author

If you REALLY prefer to shade, then you should be prefixing those packages so they don't conflict with the rest of the app. See this feature of the maven-shade-plugin: https://maven.apache.org/plugins/maven-shade-plugin/examples/class-relocation.html

@agustinona
Copy link

Hi @danieldbower ,
We apologize for this not having gained traction yet, and thank you for the valuable contribution. Our team is currently analyzing this as it would mean supporting two packages along with the associated maintenance overhead.
We expect to have a resolution for this in the following quarters.

@danieldbower
Copy link
Author

I'd recommend that if you wanted to stick with just one artifact, you go with the non-shaded version. That will remove some complexity and make the library more secure.
If you want to stay with the shaded version, please prefix those packages so they don't conflict with the rest of the app. See this feature of the maven-shade-plugin: https://maven.apache.org/plugins/maven-shade-plugin/examples/class-relocation.html

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