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

Patch PR for cleanups / enhancements in SP800-53 OSCAL including 'implemented by' and 'assurance' props #174

Closed

Conversation

wendellpiez
Copy link
Contributor

@wendellpiez wendellpiez commented Jan 7, 2023

Committer Notes

A question from a community member led me to look and what I found is a backlog of work now in a working branch, which needs to be integrated. This PR should catch us up.

Mainly, this addresses or finalizes #101 (already marked as closed) along with an earlier request for "Implemented by" and "assurance" properties (which is the real missing part). It looks like I failed to make a new branch and continued to work in a stale one?

@aj-stein-nist I can't trace the original Issue where the last item originated and was tracked (maybe in this repo or OSCAL repo). The commit with the effective changes here was made by you, because you had done a cold review of the work that I had produced, after originating the work item. Can we link to that Issue here also?

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you squashed any non-relevant commits and commit messages? [instructions]
  • Do all automated CI/CD checks pass?

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)?

@wendellpiez wendellpiez changed the title Patch PR for cleanups / enhancements including 'implemented by' and 'assurance' props Patch PR for cleanups / enhancements in SP800-53 OSCAL including 'implemented by' and 'assurance' props Jan 7, 2023
@david-waltermire david-waltermire linked an issue Jan 12, 2023 that may be closed by this pull request
Copy link
Contributor

@aj-stein-nist aj-stein-nist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wendellpiez, let's sync up tomorrow, we only merged into develop just the baseline catalog changes in the content (see #124).

I see them in develop using these links, if you Ctrl+F for "implementation-level" or "contributes-to-assurance" see below.

However, the transform and code edits were explicitly not part of that. We can should bring that in if you'd like, that definitely is worth keeping. Also, we need to rebase and make the developer branch current, can I help with that?

Can we review the file changes, I can explicitly see them already and I will have to view the diff locally to understand what's happening, but I found them easily in the browser.

Are you looking at an older develop branch on your own fork?

@aj-stein-nist
Copy link
Contributor

Hey @wendellpiez just checking in, do we need to discuss this and figure out what is or isn't getting passed in with the new props? I "see" them in certain revisions of the code and I know I am the only one who reviewed it yet, so let me know if we need to catch up on this.

@aj-stein-nist
Copy link
Contributor

Scheduled a meeting to discuss this one later in the week.

@aj-stein-nist
Copy link
Contributor

Wendell and I reviewed this in the morning and we noticed two avenues of improvement here.

  • There appears to be an important problem, perhaps a bug, in the profile resolution system within CD insomuch that the catalog in src/nist.gov/.. is copied into a level up and it appears, at least from review of this PR, that catalog, and resulting resolved catalogs from the adjacent profile, are missing the added props. So both Wendell and I are right in that the props appear to have both been added and not added at the same time.
  • There are a variety of cosmetic and editorial improvements, aside from the prop issue mentioned in the PR as the major need/ask for the change, and those are probably good (as in AJ needs to actually review them). However, we need to check the previous issue, because it would seem any changes props or otherwise, wont propagate up for the aforementioned reason (pun intended).

Copy link
Contributor

@aj-stein-nist aj-stein-nist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we consider moving out the oscal-reformat.xsl and validate-names-etc_SP800-53-catalog.sch into the usnistgov/oscal-utils repo as discussed and track it as either 1) part of usnistgov/OSCAL#1416 or 2) as a separate issue related to those efforts to split "XSLT3 core functions into util library and the OSCAL-centric ones into another" work? The latter is implied but I recall the checklist of that issue included follow up work to add issues as needed, so we ought to circle back on that. :-)

@aj-stein-nist
Copy link
Contributor

AJ agreed to set aside some time to in the next sprint, later this week or early next to week, to review and address the PR feedback.

@aj-stein-nist
Copy link
Contributor

After reviewing #204 (comment) it seems this was previously merged in a similar PR in develop, so I will close this as obsolete. Thanks again, @wendellpiez. (I think you completed that work originally in some parallel PR in any event).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Aren't these errors? (SP800-53A encoding issues)
2 participants