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

[FSSDK-10553] add IsEveryoneElseVariation marker in decide API #422

Merged
merged 8 commits into from
Aug 23, 2024

Conversation

pulak-opti
Copy link
Contributor

Summary

  • added IsEveryoneElseVariation marker in decide API

Ticket

@pulak-opti pulak-opti marked this pull request as ready for review August 21, 2024 14:26
@pulak-opti pulak-opti requested a review from a team August 21, 2024 14:27
Copy link
Contributor

@mikechu-optimizely mikechu-optimizely left a comment

Choose a reason for hiding this comment

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

I read the Jira on this, but 2 initial thoughts:

  1. Should this be solved in the SDK or via project configuration, as you mentioned as a variation?
  2. If this is something that an SDK should handle, then perhaps it needs to be put in all the base language SDKs.

There's a high probability you've already run through all these thoughts, and I'm late to understanding.

I do not see any problems with the coded solution, but will refrain from providing Approval since I'm not fully knowledgeable on the problem.

cc @jaeopt

@pulak-opti
Copy link
Contributor Author

Hey @mikechu-optimizely
Thanks for your thoughts. I have tried to implement this in the SDK first. You might have noticed the linked closed PR in SDK. But then I have changed my mind for the followings:

  • User has requested the feature for the Agent
  • Implementing this in SDK will force us to add this in other primary language SDKs to keep things in sync.
  • Users can implement the same thing on their server side in the similar way as we have done it in Agent if they need it.

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

@jaeopt
Copy link
Contributor

jaeopt commented Aug 21, 2024

@mikechu-optimizely @pulak-opti
I also see some value for adding this to all SDKs, hiding our naming rule for everyone-else.
The current implementation in agent looks reasonable for now and we can review it again for adding into the SDK level.

@pulak-opti pulak-opti merged commit 114f7c0 into master Aug 23, 2024
13 checks passed
@pulak-opti pulak-opti deleted the pulak/FSSDK-10553/everyone-else-variation branch August 23, 2024 15:18
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.

3 participants