-
-
Notifications
You must be signed in to change notification settings - Fork 579
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(metrics): audited/unaudited by policy violation state #3615
base: master
Are you sure you want to change the base?
feat(metrics): audited/unaudited by policy violation state #3615
Conversation
…, info) by total, audited and unaudited Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
72757a0
to
2eb8b7e
Compare
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesYou may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation |
@nscuro - would love to target 4.12.x milestone for this |
@nscuro - any feedback you or the team have re: this enhancement? |
@setchy I'll have a look and provide feedback by EOW. |
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.
Good enhancement overall, just needs some clarification so we can avoid breaking changes.
@Persistent | ||
@Column(name = "POLICYVIOLATIONS_WARN", allowsNull = "true") // New column, must allow nulls on existing data bases) | ||
@Column(name = "POLICYVIOLATIONS_FAIL_TOTAL", allowsNull = "true") // New column, must allow nulls on existing databases) | ||
private Integer policyViolationsFailTotal; |
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 breaking change in both the database schema (or rather the data is holds), as well as the REST API. I know it's not pretty, but we should keep using the policyViolationsFail
field and column.
We can add a new policyViolationsFailTotal
getter that just returns policyViolationsFail
- that would make the new field available via REST API without breaking semantics of the old field, and it would not require a migration of existing data.
Alternatively, we need to migrate all data from POLICYVIOLATIONS_FAIL
to POLICYVIOLATIONS_FAIL_TOTAL
, drop the POLICYVIOLATIONS_FAIL
column, and must ensure that policyViolationsFail
and policyViolationsFailTotal
return the same value in the REST API.
if (counters.policyViolationsSecurityTotal > 0) { | ||
counters.policyViolationsSecurityAudited = toIntExact(getTotalAuditedPolicyViolations(pm, component, PolicyViolation.Type.SECURITY)); | ||
counters.policyViolationsSecurityUnaudited = counters.policyViolationsSecurityTotal - counters.policyViolationsSecurityAudited; | ||
if (BooleanUtils.isTrue(violation.suppressed)) { |
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 this should check for BooleanUtils.isFalse(violation.suppressed)
instead. Suppressed findings and violations are counted separately and are not included in *Audited
metrics.
Description
Add support for audited/unaudited policy violation metrics by state (fail, warn, info)
Addressed Issue
Closes #3612
Additional Details
Checklist