-
Notifications
You must be signed in to change notification settings - Fork 19
[FSSDK-11551] Adding project config support for holdouts to go-sdk #415
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: master
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.
Pull Request Overview
This PR adds support for holdouts in the Go SDK by establishing flag dependencies on holdouts through the ProjectConfig. The change enables features to track and reference holdout IDs for proper experiment isolation and traffic allocation.
- Adds
HoldoutIDs
field to the Feature struct and related entities - Updates the feature mapping logic to populate holdout IDs from datafile
- Includes comprehensive test coverage for the new holdout functionality
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
pkg/entities/feature.go | Adds HoldoutIDs field to the Feature struct |
pkg/config/datafileprojectconfig/entities/entities.go | Adds HoldoutIDs field to the FeatureFlag struct with JSON mapping |
pkg/config/datafileprojectconfig/mappers/feature.go | Updates MapFeatures function to populate holdout IDs from datafile |
pkg/config/datafileprojectconfig/mappers/feature_test.go | Adds test coverage for holdout ID mapping functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -69,6 +69,7 @@ type FeatureFlag struct { | |||
Key string `json:"key"` | |||
ExperimentIDs []string `json:"experimentIds"` | |||
Variables []Variable `json:"variables"` | |||
HoldoutIDs []string `json:"holdoutIds"` |
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.
I think HoldoutIDS belong to Datafile not feature flag.
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.
You're absolutely right that holdoutIds don't belong in the FeatureFlag struct in entities.go - I've removed it from there.
However, there are actually two different structs:
- FeatureFlag in datafileprojectconfig/entities/ - This is for parsing the raw JSON datafile. You correctly
identified that holdoutIds don't exist in the raw datafile JSON, so I removed it. - Feature in pkg/entities/ - This is the runtime representation after processing. This is where we store computed values like HoldoutIDs after processing the holdouts section of the datafile.
The JavaScript SDK does the same thing - they compute holdoutIds at runtime and add them to the feature
representation that's returned via OptimizelyConfig API. See
https://github.com/optimizely/javascript-sdk/pull/1074/files#diff-4f5d3c88b7e0e3c2c3e3c3e3c3e3c3e3 where they compute and add holdoutIds.
So currently:
- Removed holdoutIds from FeatureFlag (datafile parsing)
- Kept HoldoutIDs in Feature (runtime representation)
- Will populate it when holdouts are parsed in the next ticket"
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.
If I am not wrong JS doesn't assign the ho ids into feature model. Runtime calculation should be inside config class.
Let's discuss offline about it.
Update ProjectConfig to establish the flag's dependency on holdouts.
Jira: https://jira.sso.episerver.net/browse/FSSDK-11551