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 external-system-nature-of-agreement #929

Conversation

DimitriZhurkin
Copy link

@DimitriZhurkin DimitriZhurkin commented Nov 22, 2024

Committer Notes

Add the external-system-nature-of-agreement constraint, which tests the nature-of-agreement values for external systems.

Related issue #907

Related PR #125

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 22, 2024 20:20
@DimitriZhurkin DimitriZhurkin self-assigned this Nov 22, 2024
wandmagic
wandmagic previously approved these changes Nov 22, 2024
Copy link

@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.

LGTM. Just needs the documentation for the allowed values then I can come back and approve 😄

@DimitriZhurkin
Copy link
Author

LGTM. Just needs the documentation for the allowed values then I can come back and approve 😄

Documentation was added last week (GSA/automate.fedramp.gov#125).

@Gabeblis
Copy link

Gabeblis commented Nov 25, 2024

Legendary. I suggest adding the help url to point to that new documentation

wandmagic
wandmagic previously approved these changes Nov 25, 2024
Gabeblis
Gabeblis previously approved these changes Nov 25, 2024
kyhu65867
kyhu65867 previously approved these changes Nov 25, 2024
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. I might put this allowed-value field next to the leveraged authorization nature of agreement field just because they are extremely similar. I also would be curious to know the difference between the two and why it is worth splitting them out, since they both have similar paths and equal allowed values. But this is non-blocking feedback.

@DimitriZhurkin
Copy link
Author

Folks,

Please do not merge this PR. In the light of my today's discussion with Brian, it'll need quite a lot of rework.

@Gabeblis Gabeblis changed the title Add external-system-nature-of-agreement [DO NOT MERGE] Add external-system-nature-of-agreement Nov 25, 2024
@DimitriZhurkin DimitriZhurkin changed the title [DO NOT MERGE] Add external-system-nature-of-agreement Add external-system-nature-of-agreement Nov 25, 2024
@DimitriZhurkin
Copy link
Author

Folks,

I've tweaked the revised Metapath. Tested every possible permutation, and everything worked correctly.

This puppy is now ready for review and merge, again.

Thanks much.

Copy link

@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.

LGTM

@DimitriZhurkin
Copy link
Author

This looks good. I might put this allowed-value field next to the leveraged authorization nature of agreement field just because they are extremely similar. I also would be curious to know the difference between the two and why it is worth splitting them out, since they both have similar paths and equal allowed values. But this is non-blocking feedback.

In #907, the latest comment by @brian-ruf states the following:
"Documentation for this issue will be addressed as part of GSA/automate.fedramp.gov#126"

I'd leave it for now.

@DimitriZhurkin DimitriZhurkin merged commit 49f152a into GSA:develop Nov 27, 2024
12 checks passed
@Gabeblis Gabeblis mentioned this pull request Nov 29, 2024
5 tasks
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.

Allowed Values for nature-of-agreement for external systems.
4 participants