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 has-marking #875

Merged
merged 4 commits into from
Nov 13, 2024
Merged

Add has-marking #875

merged 4 commits into from
Nov 13, 2024

Conversation

DimitriZhurkin
Copy link

@DimitriZhurkin DimitriZhurkin commented Nov 6, 2024

Committer Notes

  1. Add the has-marking constraint to check the existence of the <prop name="marking" value="cui"/> metadata prop.
  2. Add the allowed value for the marking metadata prop.
  3. Add the marking metadata prop to ssp-all-VALID.xml file.
  4. Add pass and fail unit test.

All Submissions:

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

@DimitriZhurkin DimitriZhurkin requested a review from a team as a code owner November 6, 2024 15:50
@DimitriZhurkin
Copy link
Author

Issue #836.

@aj-stein-gsa
Copy link
Contributor

@DimitriZhurkin why did you close this PR?

@DimitriZhurkin
Copy link
Author

DimitriZhurkin commented Nov 6, 2024

Because its workflows didn't appear on the Actions tab.

I tried recreating it locally, and all hell broke loose. Now my local repository is screwed up completely.

I reopened this PR.

@DimitriZhurkin DimitriZhurkin reopened this Nov 6, 2024
wandmagic
wandmagic previously approved these changes Nov 6, 2024
Copy link
Collaborator

@wandmagic wandmagic 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 to me, just rebase before merging

@DimitriZhurkin
Copy link
Author

I did rebase my local repo. I also did hard reset, just in case.

@DimitriZhurkin
Copy link
Author

DimitriZhurkin commented Nov 6, 2024

Paul,

How do I remove the erroneously generated | marking | entry from fedramp_extensions.feature? That entry crashes the workflows for this PR.

I tried removing marking from fedramp_extensions.feature manually, but VS Code threw the following error:
Can't push refs to remote. Try running "Pull" first to integrate your changes.

When I pull, I'm getting the following message:
Already up to date.

@DimitriZhurkin
Copy link
Author

Still the same workflow run error:

Ensuring full test coverage for "marking": OSCAL Document Constraints
Constraint marking has no tests
AssertionError: Constraint marking has no tests

@aj-stein-gsa aj-stein-gsa linked an issue Nov 7, 2024 that may be closed by this pull request
16 tasks
@aj-stein-gsa aj-stein-gsa force-pushed the add-has-marking branch 2 times, most recently from 77952e6 to 88112ff Compare November 7, 2024 03:52
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.

I have a minor wording nit, but I am happy this. I would

Copy link
Member

@Rene2mt Rene2mt left a comment

Choose a reason for hiding this comment

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

PR looks good, but waiting for suggestions to be merged and the documentation PR to be updated, then I'll approve

@aj-stein-gsa
Copy link
Contributor

aj-stein-gsa commented Nov 7, 2024

Oh and @DimitriZhurkin when you find this PR I clean up things because I misled led you. Locally on your computer if you want to make changes and not just in GitHub before moving onto the website PR requested in #875 (comment):

# I am not sure you are still on this branch right now, but just to make sure
git checkout add-has-marking
git fetch origin
git stash
git reset --hard origin/add-has-marking

Copy link

@kyhu65867 kyhu65867 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Nice work!

@DimitriZhurkin
Copy link
Author

PR looks good, but waiting for suggestions to be merged and the documentation PR to be updated, then I'll approve

Documentation created (GSA/automate.fedramp.gov#110).

@DimitriZhurkin
Copy link
Author

I have a minor wording nit, but I am happy this. I would

Wording changes implemented.

@aj-stein-gsa aj-stein-gsa merged commit 66c94cd into GSA:develop Nov 13, 2024
5 checks passed
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
None yet
Development

Successfully merging this pull request may close these issues.

Check for CUI marking on all digital authorization package content
5 participants