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

add oscal CLI validation to the pipeline #245

Merged
merged 6 commits into from
Feb 13, 2024

Conversation

wandmagic
Copy link
Contributor

@wandmagic wandmagic commented Feb 4, 2024

Committer Notes

{Please provide a brief description of what this PR accomplishes. Be sure to reference any issues addressed. If the PR is a work-in-progress submitted for early review, please include [WIP] at the beginning of the title or mark the PR as DRAFT.}

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you squashed any non-relevant commits and commit messages? [instructions]
  • Do all automated CI/CD checks pass?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you included examples of how to use your new feature(s)?

@iMichaela iMichaela requested review from nikitawootten-nist and a team February 6, 2024 01:10
build/Makefile Outdated
OSCAL_CLI_BIN:=oscal-cli
OSCAL_CLI_INSTALL_URL:=https://repo1.maven.org/maven2/gov/nist/secauto/oscal/tools/oscal-cli/cli-core/$(OSCAL_CLI_VERSION)/cli-core-$(OSCAL_CLI_VERSION)-oscal-cli.zip
OSCAL_CLI_INSTALL_PATH:=./oscal-cli
OSCAL_CLI_PATH:=$(shell which $(OSCAL_CLI_BIN) 2>/dev/null || echo $(OSCAL_CLI_INSTALL_PATH))
Copy link
Contributor

@iMichaela iMichaela Feb 6, 2024

Choose a reason for hiding this comment

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

OSCAL_CLI_PATH in environments like mine will result in the full path to oscal-cli because I have it installed ( aka /usr/local/bin/oscal-cli). It might be important to check if the already installed oscal-cli is the expected (latest) version, and not install it again in a different location.

Copy link
Contributor

@iMichaela iMichaela left a comment

Choose a reason for hiding this comment

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

@wandmagic - please see the comments and review the logic for cases when a developer has already oscal-cli installed and it is the expected version. Also - maybe the oscal-cli version can be passes as an argument and overwrite the default so we do not need to manually update the Makefile when we want to compare the output with the one of an earlier version. Just a thought.

build/Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@iMichaela iMichaela left a comment

Choose a reason for hiding this comment

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

The Makefile logic does not work if oscal-cli is already installed. The screenshot below demonstrates the point I was trying to make in the comments above. Since I have already oscal-cli installed in /usr/local/bin, and the directory is added to my PATH, the $OSCAL_CLI_PATH is set to /usr/local/bin/oscal-cli . So building the path as: $(OSCAL_CLI_PATH)/bin/oscal-cli, creates a wrong full path for oscal-cli.

Screenshot 2024-02-06 at 11 21 49 AM

@wandmagic
Copy link
Contributor Author

The Makefile logic does not work if oscal-cli is already installed. The screenshot below demonstrates the point I was trying to make in the comments above. Since I have already oscal-cli installed in /usr/local/bin, and the directory is added to my PATH, the $OSCAL_CLI_PATH is set to /usr/local/bin/oscal-cli . So building the path as: $(OSCAL_CLI_PATH)/bin/oscal-cli, creates a wrong full path for oscal-cli.

Screenshot 2024-02-06 at 11 21 49 AM

This should now be address with the changes in the latest commit, thank you for the feedback!

Copy link
Contributor

@iMichaela iMichaela left a comment

Choose a reason for hiding this comment

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

The Makefile installs oscal-cli now even if the developer has it installed already in a different location. It would be nice if it checks first if the oscal-cli exists, something like ifeq $(shell which oscal-cli), ' ') and if not null, check the version of the existing tool.
Please let me know if you want to meet on Th. to further discuss it.

@wendellpiez
Copy link
Contributor

If users start seeing validation errors in the data posted to the repository, because their tools are not up to date, what will the remedy be?

We can say 'update your tools' or we can keep the data at the old version. But why force them to update their tools (which they may not be ready to do) when we can easily provide a tool they can use?

I'd be grateful to see input on this question from other devs/stakeholders such as @nikitawootten, @aj-stein-nist and @david-waltermire-nist, as I think it's important to get this right.

@iMichaela
Copy link
Contributor

If users start seeing validation errors in the data posted to the repository, because their tools are not up to date, what will the remedy be?

We can say 'update your tools' or we can keep the data at the old version. But why force them to update their tools (which they may not be ready to do) when we can easily provide a tool they can use?

I'd be grateful to see input on this question from other devs/stakeholders such as @nikitawootten, @aj-stein-nist and @david-waltermire-nist, as I think it's important to get this right.

@wendellpiez - It might be some misunderstanding here.

The change to the Makefile proposed by @wandmagic is well aligned with the conversation we had and you very much supported of having more than one way of validating the content we are producing/releasing.

The Makefile is only used by our team to generate and validate our examples and NIST 800-53 in JSON and YAML. There is no need to bother others with this decision - this is our internal decision because we are the only ones 'consuming ' the Makefile. If you cook your dinner, you do not ask your neighbor how you should cook your stake if you know you like it rare and the neighbor is not visiting. You care how your family likes it. same with the Makefile.

The version discussed above is the version of the oscal-cli tool. The developers were suppose to use all means of validating data (the examples in this case) before releasing it. Finding a lot of validation errors in the examples we claimed are OSCAL 1.1.1 valid was not OK. @wandmagic fixed them all, and added more examples from the workshop. The errors were not formatting errors , they were constraints errors the pipeline we have is not able to catch.

I hope this makes the content of this PR more clear for you.

@wendellpiez
Copy link
Contributor

You will get no argument from me regarding how important it is to validate the data early and often. My concern is mainly with aligning tools, versions, and expectations.

There is more to discuss and I could explain more, offering potential scenarios, but it's not that important - go for it. (As a user of the tool I will just keep in mind that it's on me if the installation goes stale and I don't notice. Which is nothing more than status quo ante, except I thought that's what make installations were supposed to do for me.)

@iMichaela
Copy link
Contributor

iMichaela commented Feb 9, 2024

@wendellpiez - I agree with your comment.

@JustKuzya tested the Makefile and it worked for him even before because he did not have the oscal-cli installed. For me it failed in the previous version, and it re-installs the hardcoded version. The installation is fast and it does not add the new installation to my environment, but since the version is hardcoded, I am concerned developers will have to maintain it. If we could target the 'latest' oscal-cli release, then I would worry less. Targeting one specific version which happens today to be the latest is concerning.
This is the reason for not rushing into approving this pull request. I want our developers to have all information they need and enough control over the version they are working/testing with. And while the oscal-cli version might matter less, the oscal version the cli implements matters because it needs to match the version the examples are claiming to use. Hardcoding oscal-cli version and then forgetting what that was and not having clear message what the Makefile is doing, it might cause problems in the future.

My thinking:

  • I would like to easily run the generation and validation with and without the oscal-cli (through the invocation not by modifying the Makefile)
  • I would like the oscal-cli version and implicitly oscal version supported to be clearly reported
  • I would like to be able to define or override the oscal-cli version from the command line.
  • I would like, if I have the correct version installed, maybe to not re-install it locally. The "local installation" of oscal-cli in the ./build/oscal-cli directory is invoked with the full path from the Makefile so it will be consistently used only when the validation is invoked through the Makefile. Otherwise, any command line validation will use the installation found in the $PATH. So, if a developer is working on fixing any oscal content (examples or NIST SP 800-53) and runs the oscal-cli outside the Makefile, will use the other installation he/she has or will not find it, if not aware of the Makefile details. This can be very confusing for new team members.

Is there any item in the wishlist above that you think should be removed because it is an overkill? If "yes" - I am listening and we can cross it out. Is there anything that needs to be added? If "yes" - then let's add the item.

I assigned @nikitawootten to review as well since he is the Makefile author.

If this PR needs to be further discussed, I will release the oscal-content with the latest NIST SP 800-53 without Makefile/pipeline enhancements. The PR will have to be resubmitted.

@wandmagic
Copy link
Contributor Author

thanks for your feedback!
I'll adjust this pull request to get the latest version number from the github release api endpoint over the weekend!
I can compare to latest and local versions and only install if the version locally is not latest.

@wendellpiez
Copy link
Contributor

@iMichaela thanks, especially with others weighing in I am now more confident this is on track. Proposed solution by @wandmagic seems like is a good stopgap, at least.

I'm very curious to hear what others have to say about versions and version management across the tools, especially wrt what is the "easy path" for developers.

@wandmagic
Copy link
Contributor Author

the script will now get the latest oscal version and install it to run the validation.
still needs to detect if existing version exists and compare versions

@iMichaela
Copy link
Contributor

the script will now get the latest oscal version and install it to run the validation. still needs to detect if existing version exists and compare versions

@wandmagic - do you have an estimate for the final Makefile version? We must relate oscal-content, and I am inclined of doing so without this PR if the changes are not finished in 1-2 days.

@wandmagic
Copy link
Contributor Author

@iMichaela I updated the makefile last night, let me know if it works for you in cases where the cli is present and when it is not present, my tests showed good results. I'm happy to make a new PR to set the build job itself into to sections of build & validate and post artifact validation.

Copy link
Contributor

@iMichaela iMichaela left a comment

Choose a reason for hiding this comment

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

@wandmagic -- Please see comments above

build/Makefile Show resolved Hide resolved
build/Makefile Show resolved Hide resolved
Copy link
Contributor

@iMichaela iMichaela left a comment

Choose a reason for hiding this comment

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

Looks good now.

@iMichaela iMichaela merged commit 2225aad into usnistgov:develop Feb 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants