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

Feature: batch evaluate exclude not found #518

Conversation

itaischwartz
Copy link
Contributor

Add an option to exclude not-found flags from batch evaluate calls.

This change makes it possible to separate flags management from client calls.

  • a client can add a flag to the batchEvaluateRequest list even before the flag was added.
  • admin can remove flags w/o having to worry about batchEvaluate calls failing altogether.

@itaischwartz
Copy link
Contributor Author

@markphelps
We can think of a new version of batchEvaluate (batchEvaluteV2) where we expose a separate error value for each flag in responseList, and let the client choose how to handle the error.

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.

Great idea @itaischwartz ! Thanks for the contribution. Just one minor request re: tests.

I agree maybe a new API method is needed to add an error to each evaluation request/response.


resp, err := s.BatchEvaluate(context.TODO(), &flipt.BatchEvaluationRequest{
RequestId: "12345",
ExcludeNotFound: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

would you mind adding another test where ExcludeNotFound is false or simply not set (so it defaults to false)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that TestEvaluate_FlagNotFound is testing this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see now that it's testing another grpc call.
I'll add the missing test case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@markphelps
Copy link
Collaborator

@all-contributors please add @itaischwartz for code

@allcontributors
Copy link
Contributor

@markphelps

I've put up a pull request to add @itaischwartz! 🎉

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.

Awesome! Thanks again!

@markphelps markphelps enabled auto-merge (squash) August 7, 2021 19:15
@markphelps markphelps disabled auto-merge August 7, 2021 19:19
@markphelps markphelps merged commit cf06f4e into flipt-io:master Aug 7, 2021
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