-
Notifications
You must be signed in to change notification settings - Fork 37
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 feature flag for improved evaluation details. #4584
Conversation
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.
A few specific comments, but this approach makes sense to me.
|
||
// Option is a function that takes an evaluator and does some | ||
// unspecified operation to it, returning an error in case of failure. | ||
type Option func(interfaces.Evaluator) error |
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.
Interesting question -- do we want to have an error
return here, or do we want options to have to be successful?
I don't have a strong feeling, but I do wonder whether we want to have the error handling propagate all over the place, or have to be handled by the Option.
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.
Usually, this pattern requires the developer to apply Option
functions in some kind of constructor routine, so you'd have to handle errors anyway.
Besides, they might perform some kind of validation and the constructor should be aware of such errors.
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.
This is a philosophy question. I'm willing to go with the "there might be errors, so expose error handling" philosophy, I just wanted to point out the other path ("options must not generate errors and do something sensible themselves") so we could choose.
I think we both agree that if an option generates an error, then the underlying object that the option was applied to is in an unspecified state and must not be used.
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.
the other path ("options must not generate errors and do something sensible themselves")
I don't think that's useful generally, it might be very surprising to the user.
For example, we use options to configure the filter used in the history list
endpoint. That endpoint supports filtering by time range, and requires from
and to
to be relatively ordered in the right way (here). Not returning an error in that case would be very, very confusing to the user, who would receive results not matching his expectation.
I think we both agree that if an option generates an error, then the underlying object that the option was applied to is in an unspecified state and must not be used.
I totally agree on that, it might be in an inconsistent state.
I also add that, if all option succeed it's still up to the caller to verify that the object the options were applied to is in a consistent state.
return evalerrors.NewDetailedErrEvaluationFailed( | ||
templates.VulncheckTemplate, | ||
map[string]any{"packages": vulnerablePackages}, | ||
if e.featureFlags != nil && flags.Bool(ctx, e.featureFlags, flags.ImprovedEvalDetails) { |
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.
Should we improve flags.Bool
to handle a nil
IClient? It sort of feels better than littering nil
checks elsewhere in the code.
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 think nil
must be handled outside. If we had flags.Bool
handle the case, it would have to default to something that might be different from what set in the fags yaml file, e.g. the flags file defaults to enabled
which values to true
while flags.Bool
defaults to false
.
Since we create a single flags client and end up sharing it almost everywhere, we might as well have a single instance shared by other means wrapped inside flags.Bool
itself.
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'm okay with that, but I'd also be okay with nil
Client == false
result, always.
Needing to think about default polarity of flags in each case is probably an anti-pattern -- I'd rather (like Go) use a default-false polarity, and flags express themselves as positive assertions of some variation from the base behavior.
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 don't think reasoning about default-false polarity when writing code is the problem, I think the problems begin when trying to figure out why some user facing element, far away from the flag evaluation, does not have the expected form because of Go-style default value semantics.
In general, I'm not a big fan of default values as a solution to memory initialization either.
ef7baa1
to
8a07371
Compare
276a65f
to
661b063
Compare
This change adds feature flag `improved_eval_details`, which is currently used by the `vulncheck` evaluator to determine whether to produce "legacy" message format (i.e. comma separated list of package names) or markdown (i.e. unordered list of package names). Additionally, this change adds support for options when creating new evaluation engines, making them easier to extend and possibly test. The change is backwards compatible.
661b063
to
7d82953
Compare
Summary
This change adds feature flag
vulncheck_error_template
, which is currently used by thevulncheck
evaluator to determine whether to produce "legacy" message format (i.e. comma separated list of package names) or markdown (i.e. unordered list of package names).Additionally, this change adds support for options when creating new evaluation engines, making them easier to extend and possibly test.
The change is backwards compatible.
Change Type
Testing
Manual tests.
Review Checklist: