search

Unleash / unleash-client-rust

Unleash client SDK for Rust language projects
Apache License 2.0
23 stars 17 forks source link

fix: variant weight can be up to `1000` #55

Closed teqm closed 1 year ago

teqm commented 1 year ago

About the changes

The weight of the variant can be up to 1000, so u8 isn't enough. I couldn't find a proper reference, but it's obvious when looking at the code e.g. https://github.com/Unleash/unleash/blob/main/src/lib/routes/admin-api/project/variants.ts#L117.

I don't know when this changed (or if it always was like that) but the client specification seems to be wrong/outdated e.g. here https://github.com/Unleash/client-specification/blob/main/specifications/12-custom-stickiness.json#L24.

Important files

Discussion points

teqm commented 1 year ago

hey @sighphyre - I hate to be that guy haha, but any update on this😅? Are we blocked here by this one https://github.com/Unleash/unleash-client-rust/pull/56?

By the way, how "experimental" is this yggdrasil project? Would you advise against using it at all, or is it okay (while keeping in mind that the API may break)?

sighphyre commented 1 year ago

hey @sighphyre - I hate to be that guy haha, but any update on thissweat_smile? Are we blocked here by this one #56?

By the way, how "experimental" is this yggdrasil project? Would you advise against using it at all, or is it okay (while keeping in mind that the API may break)?

So sorry! Apparently I'm being "that guy" here, not you! I think we're still trying to figure some stuff out with how we handle builds on this project.

@rbtcollins You open force merging this? I don't have rights on this repo

sighphyre commented 1 year ago

By the way, how "experimental" is this yggdrasil project? Would you advise against using it at all, or is it okay (while keeping in mind that the API may break)?

It should be absolutely fine, we already run this in production in the Edge project. The core functionality should work fine (barring a few bugs we might have missed, I saw the PR, thank you!). We don't expect the API to change in major ways either and we'll stick to semver when we do change.

There's a ton of experimental things there but nothing in use right now, and it shouldn't be exposed to the code that evaluates the features, so if you're looking to use it, absolutely feel free

thomasheartman commented 1 year ago

@teqm Hey, wanted to chime back in here! Would you mind rebasing this off the main branch? We've finally sorted out some of the pipeline issues that were blocking the merging of this PR and would love to get it merged now.

Of course, it's a pretty minor change, but I wanna give you the opportunity to contribute instead of just "taking" your changes and running with them. Let me know! And if I don't hear anything by, say, Monday, I'll go ahead and open another PR fixing this.

Thanks again for your contribution 😄

teqm commented 1 year ago

@thomasheartman I don't really mind it being "taken" :sweat_smile: - anyway, rebased ;d

thomasheartman commented 1 year ago

Thank you! And yeah, I wasn't sure whether you'd feel short-changed or anything in case we shut this PR down and opened an identical one instead, so I thought it was safer to ask. Anyway, this looks good to me!

thomasheartman commented 1 year ago

@sighphyre Got any more input before we merge it or are we good to go?

sighphyre commented 1 year ago

@sighphyre Got any more input before we merge it or are we good to go?

Nah this looks great, thank you for all the hard work that it took to get to the point where we can merge this one!