-
Notifications
You must be signed in to change notification settings - Fork 185
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
Cherry-pick #2024 protocol port-range constraints hotfix into main
for future release
#2071
Cherry-pick #2024 protocol port-range constraints hotfix into main
for future release
#2071
Conversation
@aj-stein-gsa - Port range error has been fixed. The fix is already in the |
Yes, by definition I took that commit and cherry-picked it. I would encourage you to re-read my explanation of why I took that commit and ask you apply it to main and rebase develop accordingly for a patch release, and not push changes from |
Also, @iMichaela, no one has approved the workflow so I cannot check the tests and ensure that this PR will work. |
Please submit them against the develop branch. But the protocol range error is already fixed in develop. |
Did you read explicitly what I wrote in the PR message and the related mailing list discussion about hotfix and following NIST OSCAL release guidance? It may seem redundant, but it is important. If you are not willing to accept this PR and accept my proposed plan, please let me know. I will close this PR and plan accordingly. |
@aj-stein-gsa -- Sorry - missed that detail about cherry-picking the commit from NIST |
There are elements to my recommendation, one general, one specific.
|
Those XSLT fixes you mention were implemented and tested by @wendellpiez and @galtm and are focussing on tests we included in the CI/CD pipeline. Are you implying that FedRAMP will freez the OSCAL version used at v1.1.3, if I cherry pick from develop and release the rest right after as 1.1.4, because all fixes will be released regardless of the patch number? I am sure #2059 is of high interest as well... How come nobody cares of this bug at FedRAMP. The profile resolution PR is very old and not approved and merged for the reason you stated above. It requires a lot of reviews and tests. |
Yes, and the underlying metaschema-xslt submodule change, in addition to tests, brings in other changes to profile resolution.
I would prefer we keep comments objective and not subjective, is that OK? I am not trying to imply anything. I did not say, personally or representing FedRAMP anything about a "version freeze." We will evaluate a minimally required version as the need arises. Subjectively, It is not an issue of who or who does not care about the bug at FedRAMP. I do not want to address questions from that perspective, it is personal and does not really support you or the community if I keep framing work items and discussions that way. To directly answer your question about 1.1.3 and freezes in an objective way: if changes are useful in 1.1.3, we will evaluate, if there are separate changes in 1.1.4 we will evaluate, and determine the minimally required version for FedRAMP use from there. To not leave anything to inference and implication: I did not write the proposal nor do we plan on only using 1.1.3 and freezing the version if this proposal is followed through. We will evaluate new versions as needed to inform FedRAMP requirements. Objectively, I commented on this #2059 issue just now, and I had not reviewed it before. I commented because I see there is no bug fix in a PR or elsewhere. Please correct me if I am wrong. It would seem it is not ready for this proposal. To speak about the current state of work for the FedRAMP Automation Team, our focus is currently on SSP and then we may move onto other models. Speaking for myself and someone prioritizing that backlog, that is the only reason I have not looked upstream for issues in the AP/AR models. It is not for lack of caring or other subjective reasons. It is just not our focus right now.
I am not speaking of the unmerged PR, I am talking specifically about this submodule update. It may be minor, but I have said every time you asked, without implication, that FedRAMP does not directly use this part of the codebase and I cannot reliably test or review. That is the only reason I wanted to follow published release guidance (which has not changed and the requests, despite worthwhile explanation, deviate from it). As a courtesy, I have tried to make that clear, but I will stop. |
Any updates or changes made to profile resolution XSLTs are not in metaschema-xslt submodule, but in oscal repository. https://github.com/usnistgov/OSCAL/tree/main/src/utils/resolver-pipeline (I doubt not there are tools to compare this in different branches?) What @aj-stein-gsa is still true, however, only more so. Without regression testing for profile resolver (somewhere stuck behind As one of the devs I strongly recommend both, but if you have to pick one, not trusting the devs and seeing for yourselves is better. This goes for both profile resolution in this repo, and all |
Changes in the metaschema-xslt submodule are mostly not directly impactful (more tests), but there are meaningful changes as requested, including nominal support for In a mature system we would already have regression testing in place to ensure, e.g., we were not breaking schema production with a release. Here we have the beginnings of that. However, all the regression testing in the world does not create trust. We also need transparency and multiple assurances at the various layers. Accordingly, OSCAL also has its own nascent regression testing for schemas or some schemas (by showing me Additionally, there is also work in my demo site oscal-xproc3 framing up 'schema field testing'. Any schema in any branch or release could be targeted for testing. So the deficit here (IMHO) is not in the testing as much as it is in the replication and "V&V" (validation/verification) of the testing we have. Until we can replicate and expose the testing (across multiple test sites), we can't actually know how much of a deficit there is, to say nothing of validating new tests. |
Committer Notes
This PR is very simple and only cherry-picks a commit. This PR's commit has already been reviewed, approved, and merged into the
develop
branch, as identified by @brian-ruf in #2024 (comment) to fix #2023, would need to be "promoted" with a variety changes that may not be read for a final release, and if so, a minor or major release, not a patch release (per the NIST OSCAL Team's semantic versioning guidelines in their wiki).By accepting this cherry-picked commit, NIST OSCAL maintainers could later accept this PR, they could "re-play" after a patch release by "rebasing" the
main
orrelease-1.1
branch as prudent on develop withgit fetch origin && git checkout --track origin/develop && git pull -r origin main
(as suggested by the wiki procedures, just notdevelop
branch). A cherry-pick detection should drop the commit as already included in that branch, and this PR will facilitate quick inclusion into a possible upcoming patch release./cc @david-waltermire for awareness
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 written new tests for your core changes, as applicable?This repo does not contain model-based/instance-based testing; we will update the GSA FedRAMP Automation Team's test suite accordingly for public review and ongoing use.Have you included examples of how to use your new feature(s)?Not in this repository, but will work with GSA FedRAMP Automation Team to update our constraints and examples accordingly.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.