-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add mandatory test 6.1.48 - SSVC #358
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
base: main
Are you sure you want to change the base?
Conversation
Coverage after merging feat/197-csaf-2.1-mandatory-test-6.1.48 into main
Coverage Report
|
Coverage after merging feat/197-csaf-2.1-mandatory-test-6.1.48 into main
Coverage Report
|
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
Coverage after merging feat/197-csaf-2.1-mandatory-test-6.1.48 into main
Coverage Report
|
Coverage after merging feat/197-csaf-2.1-mandatory-test-6.1.48 into main
Coverage Report
|
Coverage after merging feat/197-csaf-2.1-mandatory-test-6.1.48 into main
Coverage Report
|
Coverage after merging feat/197-csaf-2.1-mandatory-test-6.1.48 into main
Coverage Report
|
Coverage after merging feat/197-csaf-2.1-mandatory-test-6.1.48 into main
Coverage Report
|
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
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 deep review yet as structure will change with SSVC 2.0.0 - nevertheless already some comments. Please keep them in mind when implementing the new test with SSVC v2.0.0
Wait to correct the implementation until SSVC v2.0.0 is out and in CSAF
// adapt this list if the list of registered namespaces gets extended | ||
const CURRENT_DECISION_POINTS = ['ssvc', 'cvss'] | ||
|
||
const GITHUB_TOKEN = '' |
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.
Hmm - can we do it without a GITHUB_TOKEN
? 🤔
* @param {string | undefined} namespace | ||
* @param {string | undefined} version | ||
*/ | ||
function decisionPointHash(name, namespace, version) { |
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 feels like "hash" is the wrong word here... Maybe something like "identifier"? 🤔
return ctx | ||
} | ||
|
||
const registeredSsvcNamespaces = ['ssvc', 'cvss'] |
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.
We have that multiple times in the code. Please refactor to only use it once - so there is only one place to update.
ctx.isValid = false | ||
ctx.errors.push({ | ||
instancePath: `/vulnerabilities/${vulnerabilityIndex}/metrics/${metricIndex}/content/ssvc_v1/selections/${selectionIndex}`, | ||
message: `this decision point contains invalid values or its values are not in order`, |
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.
Please split into two error messages.
No description provided.