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 scheduled-completion date, status date, and vendor check-in dates for POAM items #2117

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

aj-stein-gsa
Copy link
Contributor

Committer Notes

Add different properties as a short-term solution to missing POA&M item dates. If approved and merged, closes #2110.

All Submissions:

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

(For reviewers: The wiki has guidance on code review and overall issue review for completeness.)

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)?
  • Have you updated the OSCAL website and readme documentation affected by the changes you made? Changes to the OSCAL website can be made in the OSCAL-Pages and OSCAL_Reference repositories.

@aj-stein-gsa aj-stein-gsa changed the base branch from main to develop February 22, 2025 05:59
@aj-stein-gsa aj-stein-gsa marked this pull request as ready for review February 22, 2025 05:59
@aj-stein-gsa aj-stein-gsa requested a review from a team as a code owner February 22, 2025 05:59
@iMichaela
Copy link
Contributor

This PR requires further discussion and review. It appears to break the backwards compatibility, but I need more than few minutes to provide a robust feedback.

We might need to bring it to the OSCAL Foundation's TAG for a broader review and opinion.

NOTE for any community member that would like to review the PRs:

  1. In general, any added constraint breaks the backwards compatibility. Constraints added will make existing OSCAL artifacts that are not in a lucky situation to already do so, to become invalid.
  2. Additions of optional assemblies/fields can be accepted under a minor release since existing OSCAL artifacts will still be valid without those assemblies.

@aj-stein-gsa
Copy link
Contributor Author

This PR requires further discussion and review. It appears to break the backwards compatibility, but I need more than few minutes to provide a robust feedback.

Please do provide more feedback because I am not sure how adding only optional properties can break backwards compatibility. Can you explain this analysis when you have time for this robust feedback?

  1. In general, any added constraint breaks the backwards compatibility. Constraints added will make existing OSCAL artifacts that are not in a lucky situation to already do so, to become invalid.

That is not true if you add optional properties and only check additional properties conditionally on the existence of one optional property being defined, which is the implementation approach I used.

This analysis is incorrect.

Additionally, if I should just close these PRs and send them to the foundation copy of the OSCAL models moving forward, let me know and I will just do that instead.

@iMichaela
Copy link
Contributor

This PR requires further discussion and review. It appears to break the backwards compatibility, but I need more than few minutes to provide a robust feedback.

Please do provide more feedback because I am not sure how adding only optional properties can break backwards compatibility. Can you explain this analysis when you have time for this robust feedback?

  1. In general, any added constraint breaks the backwards compatibility. Constraints added will make existing OSCAL artifacts that are not in a lucky situation to already do so, to become invalid.

That is not true if you add optional properties and only check additional properties conditionally on the existence of one optional property being defined, which is the implementation approach I used.

This analysis is incorrect.

Additionally, if I should just close these PRs and send them to the foundation copy of the OSCAL models moving forward, let me know and I will just do that instead.

My comment indicated that the PR requires further discussion. I noted that it appears to cause the issue mentioned. I agree, and noted that, optional additions are acceptable, so we are in agreement.
I did not provide a review, I commented (courtesy comment) to acknowledge the fact that your PRs are not ignored and we will get to them as soon as feasible considering the overall work priorities. I do not review PRs by glancing at them on a cell, but I might note immediate thoughts.
If the constraints are added to an existing optional property, will cause the same validation error for people that used already that optional assembly. If the assembly is new and optional and comes with constraints, then we have no backwards compatibility issue. Note: those are general clarification comments and not statements about the PR. The PR requires cloning and a proper review.

@aj-stein-gsa
Copy link
Contributor Author

I did not provide a review, I commented (courtesy comment) to acknowledge the fact that your PRs are not ignored and we will get to them as soon as feasible considering the overall work priorities.

Ok then I want to apologize and keep it brief: let me know how I can help in review and clear up any concerns. When I PR I know I have to coordinate with you as maintainer. Just let me know, @iMichaela.

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.

poam-items should optionally encode planned and actual times of item changes and resolution
2 participants