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 all transitive dependencies #23

Merged
merged 3 commits into from Oct 11, 2022
Merged

Shaded all transitive dependencies #23

merged 3 commits into from Oct 11, 2022

Conversation

codepitbull
Copy link
Contributor

Why ?

In issue #22 I mentioned that it would be great to have transitive dependencies shaded.
The reason behind this is that posthog is an add-on and shouldn't influence the dependencies in an application which simply wants to use posthog.
Currently using posthog brings in several tranitive deps which we have been using in different versions in our projects.

What ?

To avoid dependency clashes I used the Maven-Shade-Plugin.
This moves all transitive deps into a dedicated package (com.postman.shaded) and puts them into a Uber-jar.
This DOESN'T influence how you are working with the project since this happens as part of the packaging phase so the changed packages are only ever visible when pulling the resulting jar.

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

It's a while since I've worked with Java...

Google's best practices suggest avoiding shading... https://jlbp.dev/JLBP-18

That suggests at least that we need to be super responsive to CVEs in the dependencies here. Can you suggest what we need to do to make this a good experience for users of the library (if there's more for us to do)

posthog/pom.xml Outdated
</execution>
</executions>
</plugin>
<!-- <plugin>-->
Copy link
Member

Choose a reason for hiding this comment

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

We should remove this commented code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, commented out for local testing.

I uncommented the plugin. :)

@pauldambra
Copy link
Member

@codepitbull there's also a test failing here that isn't failing locally. I guess the shading is taking us past a test timeout 🤔

@codepitbull
Copy link
Contributor Author

codepitbull commented Sep 23, 2022

Yep, not an easy decision to make :)

About my reasoning:
I treat posthog like I treat other agent tools (jaeger/senrry/...)

They should seamlessly integrate.

We currently also use Sentry, and this is the sentry-dep-tree:

+--- io.sentry:sentry:6.1.3

Zero dependencies to worry about.

The same with Jaeger (not currently in use by us, but I know their agent is shading the deps).

This is the current dep-tree of posthog:

+--- com.posthog.java:posthog:+ -> 1.0.2
|    +--- org.json:json:20210307
|    \--- com.squareup.okhttp3:okhttp:4.9.1 -> 4.10.0
|         +--- com.squareup.okio:okio:3.0.0 -> 3.2.0
|         |    \--- com.squareup.okio:okio-jvm:3.2.0
|         |         +--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.6.20 -> 1.6.21
|         |         |    +--- org.jetbrains.kotlin:kotlin-stdlib:1.6.21
|         |         |    |    +--- org.jetbrains.kotlin:kotlin-stdlib-common:1.6.21
|         |         |    |    \--- org.jetbrains:annotations:13.0 -> 22.0.0
|         |         |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk7:1.6.21
|         |         |         \--- org.jetbrains.kotlin:kotlin-stdlib:1.6.21 (*)
|         |         \--- org.jetbrains.kotlin:kotlin-stdlib-common:1.6.20 -> 1.6.21
|         \--- org.jetbrains.kotlin:kotlin-stdlib:1.6.20 -> 1.6.21 (*)

This already produced a clash wit okhttp3 and the kotlin libs. I solved this by swapping out one library and then simply having your deps updated via some resolving rules.

The issue I see is that okhttp3 is rather widely used and you might cause clashes in more applications.

About CVEs:
Well, they should already be handled by you since delegating that job to users of your lib won't really help your product.

We are currently using the OWASP-plugin to continuously check for CVEs in our dependencies. And there is also dependabot which solves this issue quite nicely.

I could provide you with a PR for also integrating OWASP into your build if you want to :)

@pauldambra
Copy link
Member

I could provide you with a PR for also integrating OWASP into your build if you want to :)

The more you want to contribute the better 💖

@pauldambra
Copy link
Member

pauldambra commented Sep 23, 2022

Just the failing test and then I'm happy to merge this 💪

@codepitbull
Copy link
Contributor Author

@codepitbull there's also a test failing here that isn't failing locally. I guess the shading is taking us past a test timeout 🤔

I just ran the tests on my mac an everything is green.

Which one is failing and could you give me the full stacktrace?

@pauldambra
Copy link
Member

Yep, they pass locally for me. Only fail in GitHub CI https://github.com/PostHog/posthog-java/actions/runs/3015694697/jobs/5028592218

I'm guessing that the timeout is too low on low-oomph runners compared to our dev machines?

@codepitbull
Copy link
Contributor Author

@pauldambra could you do me a favor and run the build on the main-branch? I have a feeling this has nothing to do with my changes since they shouldn't affect the tests at all since no deps are changed.

I will also provide the OWASP checks in this PR.

@pauldambra
Copy link
Member

Hey @codepitbull ,

We're all in Rome for an offsite this week so may be slow to respond.

Ideally, can we add a separate PR to add OWASP? Helps keep the review from spiralling and we can merge things sooner (hopefully)

I'm just creating a new PR to see how the tests are

@pauldambra
Copy link
Member

The tests pass in 7.x seconds on an unchanged branch from main https://github.com/PostHog/posthog-java/actions/runs/3128731382/jobs/5076940041

@pauldambra
Copy link
Member

Reading that code... I can't immediately see why your PR could cause these tests to fail.

The worry is that we're breaking something else and don't have a test covering the broken thing and this is a symptom of that something broken elsewhere.

(Or it's a total red herring :))

@codepitbull
Copy link
Contributor Author

Can you please allow the build-action to run again?
I increased a timeout but I primarily would like to see if it was a fluke.

@codepitbull
Copy link
Contributor Author

@pauldambra looks like that fixed it :)

@pauldambra
Copy link
Member

Sorry for the delay @codepitbull

(had to lock myself in a room to finish some other work 🤣 )

Just sorting out my permissions so I can publish the new version

@pauldambra
Copy link
Member

I'm just navigating sonatype setup so I can publish https://issues.sonatype.org/browse/OSSRH-85282

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.

None yet

2 participants