-
Notifications
You must be signed in to change notification settings - Fork 43
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
Return EvalResults from rego constraints evaluation #5204
Return EvalResults from rego constraints evaluation #5204
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.
LGTM, but I'd let others chime in as well.
@@ -152,7 +154,7 @@ func (*denyByDefaultEvaluator) parseResult(rs rego.ResultSet, entity protoreflec | |||
} | |||
|
|||
entityName := getEntityName(entity) | |||
return engerrors.NewDetailedErrEvaluationFailed( | |||
return nil, engerrors.NewDetailedErrEvaluationFailed( |
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.
Maybe just a TODO, but should we return message and entity name as an evaluation result in the future?
(We may want to think about the format here, I'm not sure)
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 was a requirement for improved evaluation details story, but the request was discussed via Slack IIRC.
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 agree, I wanted to start with the constraints engine because that's what the Trusty rule will use, but the deny-by-default engine will be updated too.
map[string]any{ | ||
"violations": srb.results, | ||
}, |
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.
In the future, should we feed the Output
from the first argument into the Template
for the EvaluationError, rather than duplicating between the two?
(That should definitely be a follow-up issue)
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.
Yes, we should also use the EvaluationError
for actual unexpected errors and the EvaluationResult
for failures.
if len(rs) == 0 { | ||
// There were no violations | ||
return nil | ||
return &interfaces.EvaluationResult{}, nil |
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.
Would it make sense to have Output: []any{}
here, or will Output being nil
be handled the same as an empty list?
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.
It will be handled the same as an empty list. I tested it in go code and in the template rendering, is there some other scenario I should check?
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.
No, this was just a question I was too lazy to check myself. 😁
Summary
Output the list of violations from the rego constraints evaluation engine.
This lets us use the violations in the
pull_request_comment
alert type.For example, when using the json violation format:
Or for the default,
text
violation format:Ref #5024
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: