-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Serialize flags and segments. #194
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
Conversation
|
|
||
| Rollout() = default; | ||
| Rollout(std::vector<WeightedVariation>); | ||
| explicit Rollout(std::vector<WeightedVariation>); |
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.
To prevent infinite template recursion.
| data_model::Flag::Rollout::Kind const& kind) { | ||
| switch (kind) { | ||
| case Flag::Rollout::Kind::kUnrecognized: | ||
| // TODO: Should we be preserving the original string. |
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 imagine all strongly typed SDKs have this problem. So I will check some others. I am not super happy with it. (Though generally I don't like having to serialize, but it doesn't seem like there is any real alternative.)
| # Inform the test harness of test service's port. | ||
| test_service_port: ${{ env.TEST_SERVICE_PORT }} | ||
| extra_params: '-skip-from ./contract-tests/sdk-contract-tests/test-suppressions.txt' | ||
| build-test: |
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 changed the build names to it is easier to scan them in the PR checks.
| } | ||
| WriteMinimal(obj, "seed", rollout.seed); | ||
| if (rollout.bucketBy.Valid()) { | ||
| obj.emplace("bucketBy", rollout.bucketBy.RedactionName()); |
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.
q: Should we be preserving this even if invalid?
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.
Maybe?
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 am fine with preserving it, because here we will have some logical value retained. Probably.
No description provided.