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 back-matter constraints resource-has-{title,rlink} #650

Conversation

Gabeblis
Copy link

@Gabeblis Gabeblis commented Sep 3, 2024

Committer Notes

Description

Added back-matter constraints and tests for each of the constraints. These should be added as they are part of the effort write all of the constraints from the constraint tracker.

Changes

  • Added constraints resource-has-title and resource-has-rlink to fedramp-external-constraints.xml.
  • Added pass and fail yaml tests for both of the above constraints.
  • Edited ssp-all-INVALID to ensure both constraints fail correctly.

{Please provide a 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 submit the PR as a draft PR using the "Draft pull request" dropdown.}

All Submissions:

By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.

@Gabeblis Gabeblis requested a review from a team as a code owner September 3, 2024 14:26
@Gabeblis Gabeblis closed this Sep 3, 2024
@Gabeblis Gabeblis reopened this Sep 3, 2024
Copy link
Contributor

@aj-stein-gsa aj-stein-gsa left a comment

Choose a reason for hiding this comment

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

Can you base this against #649? I tested this locally before rebase and got errors with the tests other than we discussed when pairing today. I will stitch than in here, but errors went away rebasing and want to make sure before we pull all this in.

Thanks in advance for doing that.

@Gabeblis Gabeblis force-pushed the constraints/back-matter/resource-has branch from 0a4b6f5 to 9033a8d Compare September 4, 2024 01:17
@Gabeblis
Copy link
Author

Gabeblis commented Sep 4, 2024

Can you base this against #649? I tested this locally before rebase and got errors with the tests other than we discussed when pairing today. I will stitch than in here, but errors went away rebasing and want to make sure before we pull all this in.

Thanks in advance for doing that.

Thanks for the review! I rebased and have no errors. All tests are passing.

@aj-stein-gsa
Copy link
Contributor

Can you base this against #649? I tested this locally before rebase and got errors with the tests other than we discussed when pairing today. I will stitch than in here, but errors went away rebasing and want to make sure before we pull all this in.
Thanks in advance for doing that.

Thanks for the review! I rebased and have no errors. All tests are passing.

Was the constraints and failing test the ones you showed me earlier today with the UUID look up to cross-ref a logo for party's logo in metadata back to a resource? Or is that work going into a separate PR to be filed later and just had issues WIPing locally on your computer?

aj-stein-gsa
aj-stein-gsa previously approved these changes Sep 4, 2024
Copy link
Contributor

@aj-stein-gsa aj-stein-gsa left a comment

Choose a reason for hiding this comment

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

Aside from my questions about the test discussed when pairing, this looks good to go from my perspective. 🚢

src/validations/constraints/content/ssp-all-INVALID.xml Outdated Show resolved Hide resolved
src/validations/constraints/content/ssp-all-INVALID.xml Outdated Show resolved Hide resolved
@Gabeblis
Copy link
Author

Gabeblis commented Sep 4, 2024

Can you base this against #649? I tested this locally before rebase and got errors with the tests other than we discussed when pairing today. I will stitch than in here, but errors went away rebasing and want to make sure before we pull all this in.
Thanks in advance for doing that.

Thanks for the review! I rebased and have no errors. All tests are passing.

Was the constraints and failing test the ones you showed me earlier today with the UUID look up to cross-ref a logo for party's logo in metadata back to a resource? Or is that work going into a separate PR to be filed later and just had issues WIPing locally on your computer?

I was working on the constraint we were discussing earlier in a local branch, and I haven't pushed anything to GitHub yet. That was the resource-is-referenced constraint, and it isn't part of this PR. I'll open a separate PR for it when I have the issue resolved.

@aj-stein-gsa
Copy link
Contributor

I was working on the constraint we were discussing earlier in a local branch, and I haven't pushed anything to GitHub yet. That was the resource-is-referenced constraint, and it isn't part of this PR. I'll open a separate PR for it when I have the issue resolved.

No worries, we can carry on convo outside of this PR review. I have approved, you get to pull the trigger to merge in. 🫡

Co-authored-by: A.J. Stein <aj@gsa.gov>
Co-authored-by: A.J. Stein <aj@gsa.gov>
Copy link
Contributor

@aj-stein-gsa aj-stein-gsa left a comment

Choose a reason for hiding this comment

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

Before final merge tomorrow, we should determine what we expect to see in the corresponding documentation. I did notice automate.fedramp.gov talks about the title for resources but doesn't say it is mandatory. Also I now see there is a little more nuance to the constraint requirements than I knew from memory per below and information about base64, rlink, neither or both being permissible in different contexts. Apologies.

https://automate.fedramp.gov/documentation/general-concepts/oscal-citations-and-attachments/

aj-stein-gsa
aj-stein-gsa previously approved these changes Sep 5, 2024
Copy link
Contributor

@aj-stein-gsa aj-stein-gsa left a comment

Choose a reason for hiding this comment

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

I will approve, but let's make sure GSA/automate.fedramp.gov#49 PR is merged along with this for docs alignment, but this is looking good.

Copy link
Contributor

@aj-stein-gsa aj-stein-gsa left a comment

Choose a reason for hiding this comment

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

Thanks for humoring the feedback and the file renames after the constraint changes.

@aj-stein-gsa aj-stein-gsa merged commit b5f103b into GSA:feature/external-constraints Sep 10, 2024
3 checks passed
aj-stein-gsa added a commit that referenced this pull request Sep 24, 2024
* Added constraints and tests for resource-has-(title/rlink)

* metapath cleanup

* Add comment

Co-authored-by: A.J. Stein <aj@gsa.gov>

* Add comment

Co-authored-by: A.J. Stein <aj@gsa.gov>

* Added or base64 condition

* Cleanup

* Edit constraint name

---------

Co-authored-by: A.J. Stein <aj@gsa.gov>
aj-stein-gsa added a commit that referenced this pull request Sep 25, 2024
* Added constraints and tests for resource-has-(title/rlink)

* metapath cleanup

* Add comment

Co-authored-by: A.J. Stein <aj@gsa.gov>

* Add comment

Co-authored-by: A.J. Stein <aj@gsa.gov>

* Added or base64 condition

* Cleanup

* Edit constraint name

---------

Co-authored-by: A.J. Stein <aj@gsa.gov>
Rene2mt pushed a commit to Rene2mt/fedramp-automation that referenced this pull request Sep 27, 2024
* Added constraints and tests for resource-has-(title/rlink)

* metapath cleanup

* Add comment

Co-authored-by: A.J. Stein <aj@gsa.gov>

* Add comment

Co-authored-by: A.J. Stein <aj@gsa.gov>

* Added or base64 condition

* Cleanup

* Edit constraint name

---------

Co-authored-by: A.J. Stein <aj@gsa.gov>
brian-ruf pushed a commit to brian-ruf/fedramp-automation that referenced this pull request Nov 8, 2024
* Added constraints and tests for resource-has-(title/rlink)

* metapath cleanup

* Add comment

Co-authored-by: A.J. Stein <aj@gsa.gov>

* Add comment

Co-authored-by: A.J. Stein <aj@gsa.gov>

* Added or base64 condition

* Cleanup

* Edit constraint name

---------

Co-authored-by: A.J. Stein <aj@gsa.gov>
This was referenced Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🚢 Ready to Ship
Development

Successfully merging this pull request may close these issues.

3 participants