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.

Sign up for GitHub

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

Jump to bottom

818 fix data center #819

Merged
merged 13 commits into from
Oct 25, 2024
Merged

818 fix data center #819

merged 13 commits into from
Oct 25, 2024

Conversation

kyhu65867
Copy link

@kyhu65867 kyhu65867 commented Oct 24, 2024 •
edited
Loading

Committer Notes

This PR addresses issue 818, thereby fixing data-center constraints and unit test files. The constraints needed to check for name=type and value=data-center, instead of name=data-center. Additionally, there were many unit tests that were not designed to test one issue at a time, (for example in the primary data center test document the country was also wrong) so I switched the documents to only one error per unit test. NOTE @Gabeblis agreed to update documentation for this PR

All Submissions:

  • Have you selected the correct base branch per Contributing guidance?
  • Have you set "Allow edits and access to secrets by maintainers"?
  • 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]
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • If applicable, does this PR reference the issue it addresses and explain how it addresses the issue?

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

@kyhu65867 kyhu65867 requested a review from a team as a code owner October 24, 2024 16:36
@kyhu65867 kyhu65867 self-assigned this Oct 24, 2024
@kyhu65867 kyhu65867 requested review from wandmagic, Gabeblis and aj-stein-gsa and removed request for wandmagic October 24, 2024 16:37
wandmagic
wandmagic previously approved these changes Oct 24, 2024
View reviewed changes
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, nice bugfix

DimitriZhurkin
DimitriZhurkin previously approved these changes Oct 24, 2024
View reviewed changes
Copy link

@DimitriZhurkin DimitriZhurkin 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. Nice XML fixes.

aj-stein-gsa
aj-stein-gsa requested changes Oct 24, 2024
View reviewed changes
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.

Great work done so quickly, mainly minor formatting things and a new style guide requirement I did not remind you of earlier on a call.

src/validations/constraints/content/ssp-data-center-US-INVALID.xml Outdated Show resolved Hide resolved
src/validations/constraints/content/ssp-data-center-US-INVALID.xml Outdated Show resolved Hide resolved
src/validations/constraints/content/ssp-data-center-alternate-INVALID.xml Outdated Show resolved Hide resolved
src/validations/constraints/content/ssp-data-center-count-INVALID.xml Outdated Show resolved Hide resolved
src/validations/constraints/content/ssp-data-center-country-code-INVALID.xml Outdated Show resolved Hide resolved
src/validations/constraints/content/ssp-data-center-primary-INVALID.xml Outdated Show resolved Hide resolved
src/validations/constraints/content/ssp-data-center-primary-INVALID.xml Outdated Show resolved Hide resolved
src/validations/constraints/fedramp-external-constraints.xml Show resolved Hide resolved
src/validations/constraints/fedramp-external-constraints.xml Show resolved Hide resolved
src/validations/constraints/fedramp-external-constraints.xml Show resolved Hide resolved
@aj-stein-gsa aj-stein-gsa linked an issue Oct 24, 2024 that may be closed by this pull request
Data Center constraints do not conform with upstream OSCAL expectations #818
Closed
1 task
@kyhu65867 kyhu65867 dismissed stale reviews from DimitriZhurkin and wandmagic via 4127747 October 24, 2024 18:24
kyhu65867 and others added 9 commits October 24, 2024 14:24
@kyhu65867 @aj-stein-gsa
Update src/validations/constraints/content/ssp-data-center-alternate-…
a21566f 
…INVALID.xml

Co-authored-by: A.J. Stein <[email protected]>
@kyhu65867 @aj-stein-gsa
Update src/validations/constraints/content/ssp-data-center-country-co…
78e54c9 
…de-INVALID.xml

Co-authored-by: A.J. Stein <[email protected]>
@kyhu65867 @aj-stein-gsa
Update src/validations/constraints/content/ssp-data-center-primary-IN…
2545939 
…VALID.xml

Co-authored-by: A.J. Stein <[email protected]>
@kyhu65867 @aj-stein-gsa
Update src/validations/constraints/content/ssp-data-center-primary-IN…
8e57658 
…VALID.xml

Co-authored-by: A.J. Stein <[email protected]>
@kyhu65867 @aj-stein-gsa
Update src/validations/constraints/content/ssp-data-center-US-INVALID…
8595809 
….xml

Co-authored-by: A.J. Stein <[email protected]>
@kyhu65867 @aj-stein-gsa
Update src/validations/constraints/fedramp-external-constraints.xml
90c2952 
Co-authored-by: A.J. Stein <[email protected]>
@kyhu65867 @aj-stein-gsa
Update src/validations/constraints/fedramp-external-constraints.xml
b2c4c20 
Co-authored-by: A.J. Stein <[email protected]>
@kyhu65867 @aj-stein-gsa
Update src/validations/constraints/fedramp-external-constraints.xml
61b8f6c 
Co-authored-by: A.J. Stein <[email protected]>
@kyhu65867 @aj-stein-gsa
Update src/validations/constraints/content/ssp-data-center-count-INVA…
30016de 
…LID.xml

Co-authored-by: A.J. Stein <[email protected]>
Gabeblis
Gabeblis previously requested changes Oct 24, 2024
View reviewed changes
Copy link
Contributor

@Gabeblis Gabeblis left a comment

Choose a reason for hiding this comment

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

This all looks good. I know you didn't make any changes to the data-center constraints on lines 37-42 in this PR, but it would be nice to add the help-url props to those as well since the documentation for those constraints is in the same exact area.

wandmagic
wandmagic approved these changes Oct 24, 2024
View reviewed changes
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

Rene2mt
Rene2mt approved these changes Oct 24, 2024
View reviewed changes
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.

Looks good to me.

@aj-stein-gsa aj-stein-gsa changed the title 818 fix data center [DONOTMERGE] 818 fix data center Oct 25, 2024
@aj-stein-gsa
Copy link
Contributor

If others who reviewed this PR are ok with the related documentation work in GSA/automate.fedramp.gov#96 and approve that, this can merge very soon. For now, I mark DONOTMERGE.

/cc @Rene2mt @wandmagic @Gabeblis @DimitriZhurkin

@aj-stein-gsa aj-stein-gsa changed the title [DONOTMERGE] 818 fix data center 818 fix data center Oct 25, 2024
@aj-stein-gsa
Copy link
Contributor

#96 is reviewed, so this is good to go. Remvoed the DONOTMERGE tag and merging in now. This is ready to ship.

@aj-stein-gsa aj-stein-gsa removed the request for review from Gabeblis October 25, 2024 13:37
@aj-stein-gsa aj-stein-gsa dismissed Gabeblis’s stale review October 25, 2024 13:38

Obsolete, help-urls added later in review.

@aj-stein-gsa aj-stein-gsa merged commit a08c9da into GSA:develop Oct 25, 2024
5 checks passed
brian-ruf pushed a commit to brian-ruf/fedramp-automation that referenced this pull request Nov 8, 2024
@kyhu65867 @aj-stein-gsa @brian-ruf
818 fix data center (GSA#819)
3113fac 
* fixed data center property arguments in ssp-all-VALID

* changed data-center constraints to point to name = type, value = data-center

* made unit tests much more specific to their file name, AKA only one error occurs in each file

* Update src/validations/constraints/content/ssp-data-center-US-INVALID.xml

Co-authored-by: A.J. Stein <[email protected]>

* Update src/validations/constraints/content/ssp-data-center-alternate-INVALID.xml

Co-authored-by: A.J. Stein <[email protected]>

* Update src/validations/constraints/content/ssp-data-center-country-code-INVALID.xml

Co-authored-by: A.J. Stein <[email protected]>

* Update src/validations/constraints/content/ssp-data-center-primary-INVALID.xml

Co-authored-by: A.J. Stein <[email protected]>

* Update src/validations/constraints/content/ssp-data-center-primary-INVALID.xml

Co-authored-by: A.J. Stein <[email protected]>

* Update src/validations/constraints/content/ssp-data-center-US-INVALID.xml

Co-authored-by: A.J. Stein <[email protected]>

* Update src/validations/constraints/fedramp-external-constraints.xml

Co-authored-by: A.J. Stein <[email protected]>

* Update src/validations/constraints/fedramp-external-constraints.xml

Co-authored-by: A.J. Stein <[email protected]>

* Update src/validations/constraints/fedramp-external-constraints.xml

Co-authored-by: A.J. Stein <[email protected]>

* Update src/validations/constraints/content/ssp-data-center-count-INVALID.xml

Co-authored-by: A.J. Stein <[email protected]>

---------

Co-authored-by: A.J. Stein <[email protected]>
This was referenced Nov 13, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aj-stein-gsa aj-stein-gsa aj-stein-gsa approved these changes

@wandmagic wandmagic wandmagic approved these changes

@Rene2mt Rene2mt Rene2mt approved these changes

@Gabeblis Gabeblis Gabeblis left review comments

@DimitriZhurkin DimitriZhurkin Awaiting requested review from DimitriZhurkin

Assignees

@kyhu65867 kyhu65867

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Data Center constraints do not conform with upstream OSCAL expectations
6 participants
@kyhu65867 @aj-stein-gsa @Rene2mt @wandmagic @DimitriZhurkin @Gabeblis