-
Notifications
You must be signed in to change notification settings - Fork 56
chore(unleash): update dependency io.getunleash:unleash-client-java to v11.0.2 (#1551) #1552
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks! Looks good to me.
thx @beeme1mr, do you have an opinion about the open questions for the tests? Maybe I'm overthinking things and it's alright to just trust the unleash dependency? |
hi @victornoel 😄
good questions, I would say adjust v1 failing test to make tests pass and add v2 tests similar to v1, and enhance them. reg. libgcc_s.so.1 used by WASM, I don't think the tests here must cover it, as testing it may require configuring and maintaining Docker containing some libraries just for tests, and here we test the provider, and not the Unleash SDK. |
fc1b19d
to
e4113e0
Compare
Thx @liran2000 :)
So actually it seems that the difference between v1 and v2 is just about the way segments are returned, and I don't think we use this at all here. Also it's kind of hard to find anything about the schema and how they differ from each other (I just found a mention of that segments thing in https://docs.getunleash.io/reference/api/unleash/get-all-client-features#responses. I concluded that it wasn't worth our energy to try to test too much things except changing the version to v2 in the json test file. I also created Unleash/unleash-java-sdk#316 in case it's not normal that the stickyness is null when returned by the Unleash SDK and added it as a comment in the test.
I will try to make some tests on my side then just to ensure all is good, but I will be away for 2 week before I can do that, so maybe we want to merge this before and see how it goes? Clearly the doc is very explicit about the fact it should just work. |
@sighphyre confirmed in Unleash/unleash-java-sdk#275 (comment) that there should be no dependencies to libgcc for sure in v11. |
I see I forgot to sign off, will do it soon :) |
Hey folks, I don't believe libgcc should be a requirement here anymore, that was on the v10 version that had a dependency on a native binary. On the change to the variant stickiness here, this is a property that we previously exposed but never should have. We're planning on removing this in the next major release and ideally we'd mark the open issue as won't-fix, depending on how much trouble it for this project. |
Hi @sighphyre, to understand better, the latest Unleash SDK version now depends on Yggdrasil, WASM, or neither ? |
The SDK does depend on Yggdrasil and the version of Yggdrasil we use is using WASM. We shouldn't need libgcc here, we compile the WASM directly into pure Java byte code so ultimately they're no WASM in the final output |
This PR
Related Issues
Fixes #1551
Notes
As far as I can see, this is not a breaking change because the only thing from unleash-client-java exposed from the open-feature Unleash provider is the configuration builder and it does not seem to have changed since v9.
To be double checked though.
The main open questions now are:
libgcc_s.so.1
Unleash/unleash-java-sdk#275 is really fixed as it seems to be