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

Spec-update-1 #42

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Spec-update-1 #42

wants to merge 3 commits into from

Conversation

rbtcollins
Copy link
Collaborator

No unit tests yet; not sure if we need them or not, but the client spec
is super thin on this.

This is primarily useful for developing new versions of the API client -
we can do a strict build where the Unleash client specification is
expected to have nothing unparsable.

Some users may wish to prevent silent failures when they upgrade and use
new features though, so this could be useful there too.
This will prevent silent merging of API conformance updates that don't
actually implement everything.
No unit tests yet; not sure if we need them or not, but the client spec
is super thin on this.
Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

So I think there's stuff missing from this (and it's also 100% spec compliant, which tells me that we really need to overhaul those spec tests). It looks like the calculation to resolve stickiness is correct but not the work to resolve the stickiness field. In the spec tests, that always seems to be customField but that's a coincidence - that field can actually be a custom field i.e. anything really.

In words, the feature works like this:

  • Stickiness is no longer limited to random, userId, sessionId or default, it can now be anything
    • In the case where random, userId, sessionId or default is not matched (e.g. stickiness is 'customField' or 'programmingLanguage' or anything really), the stickiness should search the context object for a property of that name and use the value from there

Depending on how you feel about Ruby, it might be more clear to see what the feature does from the PR that implements it there: https://github.com/Unleash/unleash-client-ruby/pull/69/files

@sighphyre sighphyre requested a review from chriswk June 27, 2022 07:32
@rbtcollins
Copy link
Collaborator Author

resolve stickiness is correct but not the work to resolve the stickiness field. In the spec tests, that always seems to be customField but that's a coincidence - that field can

so thats a super interesting choice. If I was designing the API I would absolutely not have put this in the same value location in the API as the enumeration values: because there is now a free-form string field where some values behave differently. e.g. 'default' or 'random' do not refer to context.properties['random']. Whats the right way to get review of such things into the unleash roadmap?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants