-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add new endpoint for fetching rule evaluations #2470
Conversation
cf916b8
to
61eb34d
Compare
61eb34d
to
6e63763
Compare
proto/minder/v1/minder.proto
Outdated
// If set, only return evaluation results for the named rules. | ||
// If empty, return evaluation results for all rules |
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 guess it's not intentional but the comment here is the same as the next one
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.
Oops, thanks, fixed!
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.
Is this in preparation for label-based profiles? Or something else?
yep |
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.
Are we gonna have labels be a fixed list of strings? or are we going to have labels be a keypair like k8s? This is something I'd like to get consensus on.
Yeah, that's exactl the sort of thing I wanted to highlight with this API proposal.
I was thinking bag-of-strings, but we could do keypairs if that seems more useful.
@evankanderson thinking about this further, I don't particularly have a use-case for key-value style labels. So, an array of strings is just as good in this case. We probably want to make them settable by users at some point though. Do you have thoughts on character validation for these labels? |
My first thought was to start with dns-style labels, but I'm wondering if that's too constricting. |
I'd say DNS + a minimal subset of characters like "/" and "-" should be fine to start with |
Okay, I updated the comments based on our discussion about K:V maps vs "bag of strings". We ended up going with the bag of strings approach because all our "values" ended up being singletons (e.g. "true" or "hidden", but no other value). Also clarified max size and allowed characters for the labels. |
Summary
Defines a new API for fetching rule evaluation results, rather than overloading
GetProfileStatusByName
. Does not provide an implementation, though hopefully the implementation is straightforward.Change Type
Mark the type of change your PR introduces:
Testing
Review Checklist: