-
Notifications
You must be signed in to change notification settings - Fork 195
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
Profile resolver bug fix for selected children of unselected parent #1595
Profile resolver bug fix for selected children of unselected parent #1595
Conversation
eff9ce3
to
dd6486a
Compare
Hi @galtm, thanks for submitting yet another excellent PR. We had to rebase develop and that leads to the "This branch has conflicts that must be resolved" conflicts below. I can help you with that, please let me know if and when you need that help. Thanks. |
3542193
to
ab30c7a
Compare
@aj-stein-nist : I resolved the merge conflicts. If anything doesn't look right to you, let me know. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As before, approved thanks agaltm
fb85a27
to
6661368
Compare
ab30c7a
to
273a3c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is fixing the inclusion of a parent control in the resolved profile when a child control is selected in the profile. I wonder how will this fix applicable to all security baselines in 800-53 will affect the 800-53 privacy baseline when resolved, to accurately reflect the RMF & Privacy teams' data processing and representation expectations. Suggest we discuss with the 2 teams to get a better understanding of the expectations for the privacy baseline.
f8f187f
to
21abd43
Compare
We can discuss it with them but this is a code change to the profile resolver itself to fix a bug in the resolver. How it is used for the content will only have impact when this potential change is included and the resolver is updated in the oscal-content repository, not this PR. Am I missing something in that regard? |
Will rebase on rebased |
Unselected parent could have multiple children that are selected, so data type of template must accommodate multiple elements.
273a3c4
to
7579e24
Compare
Generally speaking, no, you are not missing anything related to this PR. The PR looks good. I asked because I considered the code fix being merged and used by us to resolve the SP 800-53 security and privacy baselines, with the privacy baseline being a different 'beast' than the security ones, and I wonder what is that we need to be aware of when we need to update the baselines soon. |
We already have published baselines so I am not understanding your concern sufficiently about next steps. Aside from the PR, we should set aside time during our next sync to discuss this point, @iMichaela. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before approval, I will want to create a cross-reference issue to include for an upcoming patch release to identify this fix. In the interim, I will approve it as the work continues to be solid.
Thanks again, @galtm, and I appreciate your patience.
Committer Notes
This PR fixes a profile resolver bug I noticed, where a certain XSLT template needs to declare its return data type
as="element(o:control)*"
instead ofas="element(o:control)?"
. An unselected parent can have multiple children that are selected, so the data type must accommodate multiple elements.All Submissions:
"?
By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.