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

feat: add evalution reason #1099

Merged
merged 6 commits into from
Oct 28, 2022
Merged

feat: add evalution reason #1099

merged 6 commits into from
Oct 28, 2022

Conversation

markphelps
Copy link
Collaborator

@markphelps markphelps commented Oct 27, 2022

Re: #1075

Adds reason field (enum) to EvaluationResponse to provide more info about why a request matched or not

ex:
Screen Shot 2022-10-27 at 3 52 23 PM

Screen Shot 2022-10-27 at 4 05 57 PM

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2022

Codecov Report

Merging #1099 (ba31aae) into main (3c6bd20) will increase coverage by 0.34%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main    #1099      +/-   ##
==========================================
+ Coverage   80.52%   80.86%   +0.34%     
==========================================
  Files          26       26              
  Lines        1884     1897      +13     
==========================================
+ Hits         1517     1534      +17     
+ Misses        287      285       -2     
+ Partials       80       78       -2     
Impacted Files Coverage Δ
internal/server/evaluator.go 93.60% <85.71%> (+1.90%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@markphelps markphelps changed the title feat(wip): add evalution reason feat: add evalution reason Oct 27, 2022
@markphelps markphelps marked this pull request as ready for review October 27, 2022 20:47
Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Solid. I added one clarifying question. Which doesn't need to block this PR.

Comment on lines +74 to +79
enum EvaluationReason {
UNKNOWN_EVALUATION_REASON = 0;
FLAG_DISABLED_EVALUATION_REASON = 1;
FLAG_NOT_FOUND_EVALUATION_REASON = 2;
MATCH_EVALUATION_REASON = 3;
ERROR_EVALUATION_REASON = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: This is something I think we can standardise on.

In my first pass at the auth protobufs, I did what you have here.
I put <NAME>_<TYPE> for each enum.

The output in Go is stuff like flipt.EvaluationReason_UNKNOWN_EVALUATION_REASON.
Which is a bitter stuttery.

So, I dropped the _<TYPE> bit. And now the Go types are like flipt.EvaluationReason_UNKNOWN.

Is this something we should embrace more? or keep it consistent with what we have.
I don't feel super strongly either way. I like the brevity, but I also appreciate consistency.
It is easier for me to repeat this pattern in the long-lived branch, than us attempt to change the decisions of the past.

Copy link
Collaborator Author

@markphelps markphelps Oct 28, 2022

Choose a reason for hiding this comment

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

yeah, that also bothers me (the suttering names). I wonder if there is a way to alias the existing enums to be less verbose and start anew here with this pattern? I'd like to be consistent whichever path we choose though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It actually seems like we can add aliases for enums in protobuf:

https://developers.google.com/protocol-buffers/docs/proto3#enum

You can define aliases by assigning the same value to different enum constants. To do this you need to set the allow_alias option to true, otherwise the protocol compiler will generate an error message when aliases are found. Though all alias values are valid during deserialization, the first value is always used when serializing.

enum EnumAllowingAlias {
  option allow_alias = true;
  EAA_UNSPECIFIED = 0;
  EAA_STARTED = 1;
  EAA_RUNNING = 1;
  EAA_FINISHED = 2;

I might give this a shot and see if its gross or not

Copy link
Contributor

Choose a reason for hiding this comment

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

At the very worst, we might have just a small set of anomaly enums. An alias would be useful.
There are aliases, I believe, but I think buf lint doesn't like them:
https://docs.buf.build/lint/rules#enum_no_allow_alias

We can ignore that, but it says the JSON serialization format suffers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

actually looking at https://docs.buf.build/lint/rules#enum_value_prefix, there's a reason that we add the _TYPE to the proto as we wouldn't be able to have multiple UNKNOWN (for example) enum values in the same 'package'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just went with a suffix instead of a prefix it seems

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh interesting. I would've wrongly assumed protobuf could namespace the values of the enum by type automatically on the serialized form.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will update the authorization PR to bring it inline.

@markphelps markphelps enabled auto-merge (squash) October 28, 2022 16:08
@markphelps markphelps merged commit 967855b into main Oct 28, 2022
@markphelps markphelps deleted the evaluation-reason branch October 28, 2022 16:14
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