-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: implement semver strategy constraints #65
base: main
Are you sure you want to change the base?
Conversation
250a119
to
60b6167
Compare
@@ -1,7 +1,7 @@ | |||
version: "3.3" | |||
services: | |||
web: | |||
image: unleashorg/unleash-server:4.12.6 | |||
image: unleashorg/unleash-server:4.16.0 |
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.
This version adds constraints.
See https://docs.getunleash.io/how-to/how-to-add-strategy-constraints
@@ -800,9 +799,10 @@ where | |||
Err(_) => { | |||
warn!("poll: failed to memoize features"); | |||
} | |||
}, | |||
Err(err) => { | |||
warn!("poll: failed to retrieve features {}", err); |
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.
This log was super useless, as it gave 0 information on why this failed. Related issue: #35
60b6167
to
fef9e78
Compare
README.md
Outdated
@@ -107,4 +107,4 @@ UNLEASH_API_URL=http://127.0.0.1:4242/api \ | |||
|
|||
or similar. The functional test suite looks for a manually setup set of | |||
features. E.g. log into the Unleash UI on port 4242 and create a feature called | |||
`default`. | |||
`default` & `semver` with a `SEMVER_EQ` strategy constraint. |
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.
Imo it would make more sense to mock this using https://github.com/alexliesenfeld/httpmock for example, than having to do manual steps to run tests.
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.
Thats the functional test suite, which exists deliberately to check consistency with the actual separate publish Unleash API server code, which varies and changes independently. There are unit tests separate to that.
fef9e78
to
9e1acb0
Compare
@sighphyre @thomasheartman is there any plan to merge/release this, or #27 in near future? This is currently blocking us from using unleash in a rust project. Can publish a new crate to unblock, but really don't want to do that for a feature that could benefit everyone. |
@Meemaw I think we'd love to get this out, yeah. I don't know exactly what happened to @sighphyre's PR from a year ago (#27), but there is probably a reason why it wasn't merged. Maybe either he or @rbtcollins could shed some light on that. I don't know what's holding us back (it might just be development time), but I know this repo is still using a client spec from three years ago. I've got a feeling that we'd also want to update that when moving forwards, but I'm not sure it's necessary before merging this. As you can maybe tell, I don't have as much context around this particular SDK as @sighphyre and @rbtcollins do, so I'll defer to them. |
Hey @Meemaw, there's no concrete reason we haven't moved on that, it was just a case of other work becoming more important and remaining more important until right now. To date we haven't had anyone ask for these features in the Rust SDK and other SDK work was prioritised Now that people are asking it can be reprioritized but I probably won't look at least until early next week |
@sighphyre thanks for the context. I think it wouldn't be as bad if the SDK worked and just ignored those constraints and treat them as a noop, but the fact that it doesn't work at all with these constraints in the project makes it very hard to use. I'll check in again next week to see if you can prioritise this. |
Hey @sighphyre as promised circling back to check if there is a plan to prioritise this soon/this week? If not that's fine and we will work-around the issue for now. |
Hey @Meemaw, still planning to prioritise this soon but it might not be this week. I have some relatively urgent things to work on in another SDK, as soon as that's done this will be the next thing. |
About the changes
Implements semver based strategy constraints.
Closes #64
Closes #35
There a re a bunch of other constraints still not implemented, that should be trivial to add in a followup. For now, this PR has a bit more logic due to the semver constraint using the "value" field instead of "values" which requires serde to be aware of that.
Edit: I see there is a ~1 year old PR (#27) that implements constraints as well. Wondering what's up with that.
Important files
Discussion points