Skip to content

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

Conversation

sshiells-scottlogic
Copy link
Contributor

No description provided.

@sshiells-scottlogic sshiells-scottlogic requested a review from a team as a code owner April 27, 2025 18:48
Copy link

netlify bot commented Apr 27, 2025

Deploy Preview for common-cloud-controls canceled.

Name Link
🔨 Latest commit 87ace06
🔍 Latest deploy log https://app.netlify.com/sites/common-cloud-controls/deploys/681cc5f8f1d6780008641e39

@sshiells-scottlogic sshiells-scottlogic changed the title Update validation to use validate against latest schema feat: Update validation to use validate against latest schema Apr 27, 2025
Copy link
Contributor Author

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).

@sshiells-scottlogic sshiells-scottlogic requested review from a team as code owners April 27, 2025 19:22
@sshiells-scottlogic
Copy link
Contributor Author

@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?

image

image

@eddie-knight
Copy link
Collaborator

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:

https://github.com/finos/common-cloud-controls/tree/9e049c42aa08a0a313a47aa65cb68c84c3b04998/services/ai-ml/mlde

@sshiells-scottlogic
Copy link
Contributor Author

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?

@@ -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
Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

@eddie-knight eddie-knight May 1, 2025

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be as simple as

image

So we just need to decide where we need limits and what we want them to be.

@sshiells-scottlogic
Copy link
Contributor Author

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:

https://github.com/finos/common-cloud-controls/tree/9e049c42aa08a0a313a47aa65cb68c84c3b04998/services/ai-ml/mlde

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.

Copy link

github-actions bot commented Jun 7, 2025

This issue will be closed as stale in 7 days. If this issue is blocked,
please tag or assign the appropriate party to move this forward.

@github-actions github-actions bot added the stale label Jun 7, 2025
Copy link

Closed as stale. An update may reopen this PR.

@github-actions github-actions bot closed this Jun 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants