-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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(custom-checks-metadata): add new fields #3976
feat(custom-checks-metadata): add new fields #3976
Conversation
"Severity", | ||
"CheckTitle", | ||
"Description", | ||
"Risk", | ||
"RelatedUrl", | ||
"Remediation", | ||
], |
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.
Do we want to mark all as required? I think it should be optional. What do you think?
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 they should be required. If missing, then the values from the check's json should 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.
Yes, I totally agree with that.
You can check the documentation for this PR here -> SaaS Documentation |
You can check the documentation for this PR here -> SaaS Documentation |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3976 +/- ##
========================================
Coverage 86.56% 86.56%
========================================
Files 768 777 +9
Lines 23892 24155 +263
========================================
+ Hits 20682 20910 +228
- Misses 3210 3245 +35 ☔ View full report in Codecov by Sentry. |
You can check the documentation for this PR here -> SaaS Documentation |
You can check the documentation for this PR here -> SaaS Documentation |
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.
Great job @pedrooot 👏 I left some comments to verify the logic.
"required": [ | ||
"CLI", | ||
"NativeIaC", | ||
"Other", | ||
"Terraform", | ||
], |
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 sure of this? I think we want just to override the fields present in the custom metadata file.
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.
You are true, I think no field should be required.
"type": "string", | ||
}, | ||
}, | ||
"required": ["Text", "Url"], |
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 sure of this? I think we want just to override the fields present in the custom metadata file.
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.
You are true, I think no field should be required.
"Code": { | ||
"type": "object", | ||
"properties": { | ||
"CLI": { |
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.
Can you add some tests to modify for example the Remediation.Code.CLI
and check that the rest of the original check's metadata is kept.
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.
Done, also I've modified the update_check_metadata
method since It wasn't working fine for sub-fields.
You can check the documentation for this PR here -> SaaS Documentation |
You can check the documentation for this PR here -> SaaS Documentation |
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.
🚀
Context
Fixes #3923
Custom checks metadata are useful to assing the needed values for metadata.
Description
In this pr new fields are added for custom checks metadata:
Thanks @jchrisfarris for the recommendation
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.