-
Notifications
You must be signed in to change notification settings - Fork 59
feat: Update validation to use validate against latest schema #733
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: Update validation to use validate against latest schema #733
Conversation
✅ Deploy Preview for common-cloud-controls canceled.
|
.vscode/settings.json
Outdated
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.
@eddie-knight just an FYI - this started to flag validation issues in the UI for in the common files (which have a different file structure).
@eddie-knight I think this should have the inline comment identifiers back in place. If you spot any that are missing then there is something wrong with the new validation code. There are some errors left, somehow ai-ml/mlde has controls for mlde threats, but no threats in the file? |
Yeah, that's a long-standing error — it seems we merged in the controls without the accompanying threats a while back. See the files in the commit prior to this merge: |
So how do we fix this? For me it doesn't really make sense (in the context of the project) to have controls for threats that don't exist? @mlysaght2017 any thoughts? |
services/crypto/key/controls.yaml
Outdated
@@ -69,7 +69,8 @@ control-families: | |||
threat-mappings: | |||
- reference-id: CCC | |||
identifiers: | |||
- CCC.KeyMgmt.TH01 # Deletion or disabling of KMS key versions leading to denial of service or data destruction |
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.
@eddie-knight does this look like a reasonable way to split long comments to pass linting? I have used the '|' as a separator so the validator knows to check the next line.
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.
Reasonable, absolutely.
Though — if we can manage it — I think I'd prefer to enforce a max character length for titles
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.
What length would you like and can you suggest an alternative title for this one and I'll try to make it happen.
I'd likely do that as a separate PR just to keep things cleaner.
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 recall the error this was trying to solve, but I suppose it'd be something beneath that
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.
Just an FYI we discussed this on the call and we decided to flag these as warnings as they are not a priority to fix so I have adjusted the validatior to handle this case. |
This issue will be closed as stale in 7 days. If this issue is blocked, |
Closed as stale. An update may reopen this PR. |
No description provided.