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

Batch evaluate ignore disabled flags #376

Merged
merged 4 commits into from
Jan 15, 2021

Conversation

vic3lord
Copy link
Contributor

When using BatchEvaluate the entire call is canceled with an error once it encounters the first disabled flag.
In order to be able to disable a flag without failing the entire request I added a new ErrDisabled and checked when doing BatchEvaluate.

This way you can continue batching flags with the possibility to disable flags.

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

makes sense @vic3lord , would you mind adding a test to validate the new behavior? also one comment/question

server/server.go Show resolved Hide resolved
@markphelps
Copy link
Collaborator

Thanks @vic3lord! I'll create a new release this weekend. I'm wondering if we should just not return errors when trying to evaluate a disabled flag in general (even for a single/non-batch evaluate call)?

@markphelps markphelps merged commit e42da21 into flipt-io:master Jan 15, 2021
@vic3lord
Copy link
Contributor Author

vic3lord commented Jan 15, 2021

I agree, we can discuss this further but I wanted to be conservative since you are the maintainer, I didn't want to assume design decisions on my own.

We use flags internally as kill switches for new features that are not UI A/B testing.

Might make sense to not return an error in this case.

Thanks again for merging

@vic3lord vic3lord deleted the batch-evaluate-better-error branch January 16, 2021 19:13
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.

2 participants