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

Catalog constraints added in oscal_catalog_metaschema.xml - see issue #1949 #1952

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

iMichaela
Copy link
Contributor

Committer Notes

Added metaschema constraints for catalog/group/part/@name to accept "statement" and for catalog/group/control/part/@name to accept "example". These changes allow more catalogs, frameworks (e.g. CSF v2) to be represented in OSCAL without extensions and ns not being able to be processed by GRC tools without a registry.

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 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.

@iMichaela iMichaela requested a review from a team October 24, 2023 00:36
Copy link
Contributor

@david-waltermire david-waltermire left a comment

Choose a reason for hiding this comment

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

As a community member, I don't think this PR is ready. Comments inline.

src/metaschema/oscal_catalog_metaschema.xml Outdated Show resolved Hide resolved
…traint for group/part in oscal_catalog_metaschema
Copy link
Contributor

@wendellpiez wendellpiez left a comment

Choose a reason for hiding this comment

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

This breaks backward-compatibility for anyone who might be using the value 'statement', which is no longer permitted.

Please consider adding new value(s) but not removing any values already enumerated, at least until a 2.0 version.

@iMichaela
Copy link
Contributor Author

iMichaela commented Nov 29, 2023

This breaks backward-compatibility for anyone who might be using the value 'statement', which is no longer permitted.

Please consider adding new value(s) but not removing any values already enumerated, at least until a 2.0 version.

@wendellpiez The value statement for group/part@name was proposed by me in the initial PR and contested by Dave (see his comments). I followed his suggestions and replaced it with instruction. It is not in our current schema. Can you please elaborate on where do you see a backwards compatibility break?

@wendellpiez
Copy link
Contributor

If the value is not in the current schema (and only being removed from the file in the branch), I retract my objection. Adding new values to the reserved namespace is fine.

I'm not super keen on 'instruction' however either - what is the requirement being addressed by it? Since users are free to mark parts in their own namespace however they like, or leave parts with no name. Is there a specific behavior to be attributed for the 'instruction' that could not be met, for example, by

<part>
  <title>Instructions</title>
  ...
</part>

which is already valid?

These are not blocking questions, but I ask because afaik they have not been addressed.

FWIW, not having a registry is not in itself a reason to add new values to enumerated lists. They still incur costs to downstream developers.

@Compton-US Compton-US dismissed wendellpiez’s stale review November 29, 2023 18:37

Discussed and addressed.

@Compton-US Compton-US requested review from wendellpiez and a team November 29, 2023 18:44
Copy link
Contributor

@wendellpiez wendellpiez left a comment

Choose a reason for hiding this comment

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

lgtm

@Compton-US Compton-US merged commit 290bc84 into develop Nov 29, 2023
1 check passed
Compton-US pushed a commit that referenced this pull request Dec 6, 2023
…1949  (#1952)

* Two additional allowed values for catalog/group/part/@name and catalog/group/control/part/@name

* aligned the description of group/part@name='statement' and control/part@name='statement'

* Fixed typo in the oscal_ssp_metaschema and updated controversial constraint for group/part in oscal_catalog_metaschema

* Update src/metaschema/oscal_catalog_metaschema.xml

Fixed grammar.

Co-authored-by: Chris Compton <[email protected]>

---------

Co-authored-by: Iorga <[email protected]>
Co-authored-by: Chris Compton <[email protected]>
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.

5 participants