Conversation
galtm
left a comment
There was a problem hiding this comment.
Some annotations to make large diffs more manageable
src/utils/util/resolver-pipeline/oscal-profile-resolve-finish.xsl
Outdated
Show resolved
Hide resolved
src/utils/util/resolver-pipeline/oscal-profile-resolve-merge.xsl
Outdated
Show resolved
Hide resolved
| <xsl:template match="catalog[merge/as-is=$true-content]" priority="12" as="element(catalog)"> | ||
| <!-- Template return value is element(catalog) possibly preceded by a | ||
| processing instruction from detect-multiple-structuring-directives. --> | ||
| <xsl:template match="catalog[merge/as-is=$true-content]" priority="12" as="item()+"> |
There was a problem hiding this comment.
Example of the kind of data type change required by Item 3 in PR description.
src/utils/util/resolver-pipeline/oscal-profile-resolve-merge.xsl
Outdated
Show resolved
Hide resolved
src/utils/util/resolver-pipeline/oscal-profile-resolve-modify.xsl
Outdated
Show resolved
Hide resolved
...ifications/profile-resolution/profile-resolution-examples/nonfatal-error-warning_profile.xml
Outdated
Show resolved
Hide resolved
|
@david-waltermire-nist , @wendellpiez : I'm a little concerned about potential conflicts between this PR and #1424 (which I haven't looked at in detail, but I see some of the same files). If I can help make file merges go more smoothly, let me know. |
wendellpiez
left a comment
There was a problem hiding this comment.
Thanks again for all the tremendous work here, it's looking nice!
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. |
|
Hi, @aj-stein-nist . Thanks for notifying me and offering to help. The files listed are files that I haven't touched, which should make it easier. I'll try to look into this today or tomorrow and I'll keep you posted. |
e00cbaf to
dcfdcdb
Compare
|
@aj-stein-nist : I resolved the merge conflicts. If anything doesn't look right to you, let me know. Thanks. @wendellpiez : When I did the force-push, GitHub "dismissed" your earlier code review. |
We should figure out how to move forward with this given your concerns. Would it make sense to move this PR against #1424? |
Would that mean that this PR can't be merged until #1424 is merged? #1424 is a draft and has been open for about 5 months. To keep things moving, can you merge this change onto develop and rebase #1424 on top? If I could understand the essence of what is changing with #1424 I could look into whether there are any potential merge conflicts with this PR that might be hard to resolve. However, I don't know how to make sense of the 56 commits that probably resulted from rebasing the develop branch. Which commits should I be looking at in that list? |
I think your suggestion of merging this now and rebasing #1424 on top is a good path. |
wendellpiez
left a comment
There was a problem hiding this comment.
Beautiful work here, and much interesting progress.
31b54f0
|
2/4/23: Pushed 2 commits to fix minor issues I noticed while working on something unrelated. |
|
3/9/23: Manually resolved merge conflicts now that #1685 has been merged into develop branch. |
| </x:expect> | ||
| <x:expect label="The target control does not appear" test="empty($x:result//*[@id='a1-stmt'])"/> | ||
| </x:scenario> | ||
| <x:scenario label="Explicit binding to a param that needs to be set *and* have new params inserted"> |
There was a problem hiding this comment.
@wendellpiez : Does this test scenario cover the situation in issue #1859?
There was a problem hiding this comment.
I don't think so ... what is being described is wanting to add a new prop to an existing param, but not being able to use alter. Apparently alter is a no-op against params. set-param is the available workaround (it can be used to add new content). But the spec does not say that you can't target param elements using alter/add, so that should also work.
There was a problem hiding this comment.
Thanks. You're right, this scenario isn't the same as what the issue is about. But I think the XSLT in this PR fixes the issue. If I create a similar test scenario with
<alter control-id="a1">
<add position="starting" by-id="a1_prm1">
<prop ns="https://fedramp.gov/ns/oscal" name="param-label" value="AC-1(a)"/>
</add>
</alter>
then I can see that the new prop gets added at the beginning of the existing param, like this:
<param id="a1_prm1">
<prop ns="https://fedramp.gov/ns/oscal" name="param-label" value="AC-1(a)"/>
<label>A1 Parameter 1</label>
<constraint>at least every 3 years</constraint>
</param>
Should I add the new test scenario?
There was a problem hiding this comment.
Sure! I guess I was not looking closely enough, or not looking at the current code, to see that this should actually work.
If it actually works, so much the better, we only need to be able to show that it does.
fb85a27 to
6661368
Compare
5d3b9b3 to
30e81c9
Compare
30e81c9 to
0825b9f
Compare
|
Well, this is frustrating, I have a backup of this branch before rebase, but I am unable to push to your fork @galtm after "aligning" it to develop (with @galtm, can you check out my branch and push it here to reopen? Shorthand instructions below. Thanks. I was trying to spare you the effort of rebasing this yourself. # Just for alignment on what remotes I mean
git add remote me git@github.com:galtm/OSCAL.git
git add remote aj git@github.com:aj-stein-nist/OSCAL.git
git fetch all
git checkout --track aj/profiler-error-handling-backup
git push me profiler-error-handling-backup:profiler-error-handling -fThen you should be able to continue on this PR by ID and receive the credit you deserve. Apologies. |
|
@aj-stein-nist : I tried to follow your shorthand instructions, but I'm not sure if things worked. Here's the output I got from the push: where origin is as follows (the Despite the lack of error messages, this PR page still says 0 commits. I thought it would add commits to this PR but they'd be aligned with where It may be simpler for me to sync my develop branch, create a fresh branch from that, do a file-merge (outside Git) of the profile resolver file contents based on the files in a backup branch, and try again to push to the branch of this PR. I started doing that but still need to check the results. I'll be busy for a few hours, but I will come back to this in the late afternoon. |
|
Well now I am confused because it does look like you "put it back" to the way it was from my backup if I simulate what a PR would like, so now I am scratching my head. |
|
It seems now I can reopen? |
|
Well ti seems even though I have reopened it and commented it seems to ignore the state of the branch on your end even though it looks just as before (155 commits off of develop and the same series of changes I worked through last night). I guess I will try and reopen a new PR, @galtm. I sincerely apologize this mishap is frustrating and embarrassing on my end! |
|
I'm confused, too, because I thought rebasing against develop would mean that the list of commits and changed files would look like my work in isolation rather than the long list that I see in #1943. I also realized that the code needs a few tweaks because the profiler resolver code is less deep in the directory hierarchy than it used to be. I created #1944 with what I think is the right code, on top of the develop branch. Does it look right from your perspective? |
I am looking at in a few minutes, we just wrapped our sprint review, thanks again @galtm. |
Committer Notes
This PR enhances the XSLT profile resolver so that (a) if there is a fatal error, the resolver reports one such error and does not produce a final output, and (b) if there are no fatal errors, the resolver reports all nonfatal error and warning messages when producing the final output document.
Included bug fixes for bugs I noticed during this work:
All Submissions:
"?
By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.
Changes to Core Features:
Have you included examples of how to use your new feature(s)?Have you updated all OSCAL website and readme documentation affected by the changes you made? Changes to the OSCAL website can be made in the docs/content directory of your branch.Testing Done
While qualifying these changes, I ran commands like the following in a Windows shell, from the
resolver-pipelinedirectory.The command window output shows the error message as in the following example.
In another command-line example with nonfatal errors and warnings, the command window output includes the messages near the end:
Behavior when using Oxygen transformation scenario: Messages appear in the Messages pane. (In the case of fatal error messages, this pane is underneath the Transformation problems pane.)