Skip to content
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

fix: error checking prior to file writes #687

Merged
merged 6 commits into from
Oct 1, 2024

Conversation

meganwolf0
Copy link
Collaborator

Description

Adds functionality to validate if the desired output files for validate and compose are valid OSCAL models. Also a few additional error detail.

Related Issue

Fixes #678

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@meganwolf0 meganwolf0 marked this pull request as ready for review September 27, 2024 13:19
@meganwolf0 meganwolf0 requested a review from a team as a code owner September 27, 2024 13:19
Copy link
Member

@brandtkeller brandtkeller left a comment

Choose a reason for hiding this comment

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

I'm looking at some slight restructuring of the OSCAL pkg as is - Nonetheless I believe your adjustment to validate nails the intent of the (current) discussion in the issue -> checking output validity before executing any processing logic.

Do we want to map this behavior to all locations that would merge processed data with an existing file?

@meganwolf0
Copy link
Collaborator Author

Do we want to map this behavior to all locations that would merge processed data with an existing file?

Sure, I think the only one that would be missing is generate? Did I miss something else?

@brandtkeller
Copy link
Member

Do we want to map this behavior to all locations that would merge processed data with an existing file?

Sure, I think the only one that would be missing is generate? Did I miss something else?

Thinking about the paths now - otherwise I think the only distinction I would make is repeating the pattern of checking for validity of the output file before processing - so scenarios like evaluate might need checking up-front as well.

@meganwolf0 meganwolf0 marked this pull request as draft October 1, 2024 19:59
@meganwolf0
Copy link
Collaborator Author

so scenarios like evaluate might need checking up-front as well.

It looks like evaluate sort of already has this built in - the first thing it does is grab all the files passed in and convert them to assessment-results, which should basically act as a validation of the output (since the output file is the input file)?

@meganwolf0 meganwolf0 marked this pull request as ready for review October 1, 2024 20:13
Copy link
Member

@brandtkeller brandtkeller left a comment

Choose a reason for hiding this comment

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

Talked through how evaluate actively performs this functionality to-date. Covers known paths where a merge operation is expected and handles the check before performing any processing cycles

@brandtkeller brandtkeller merged commit 1ab0eef into main Oct 1, 2024
5 checks passed
@brandtkeller brandtkeller deleted the 678-error-checking-prior-to-file-writes branch October 1, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Error checking prior to file writes
2 participants