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

Create negative examples folder #270

Closed
wants to merge 1 commit into from
Closed

Conversation

davaya
Copy link

@davaya davaya commented Jul 8, 2024

Committer Notes

This PR establishes a folder to hold examples that are currently valid (as of OSCAL v1.1.2) but illustrate problems that should be fixed in future versions. It includes the first such example, an Assessment Plan illustrating a self-cycle in the tasks field that permits recursive data, contradicting the goal of models being DAGs.

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

This PR does not propose a core feature of OSCAL, it proposes an update to the CI/CD process. The user story is:

  • OSCAL maintainers modify CI/CD checks to validate anti-patterns and tag those that do not fail validation as issues to be addressed in future versions
  • OSCAL maintainers receive a proposed anti-pattern example and determine if it should not be accepted as a valid OSCAL file.

@davaya
Copy link
Author

davaya commented Jul 10, 2024

There has been discussion of why "recursive fields" is an anti-pattern, using the Assessment-plan/tasks field as an example.

  1. Assume that an assessment plan model has been designed and it involves a set of tasks. Each Task started with its current set of fields except for uuid and tasks.
  2. Somebody wrote a user story saying "I need to be able to reference a task independently of the AP that defines it". Please add a uuid field that is "A machine-oriented, globally unique identifier with cross-instance scope that can be used to reference this task elsewhere in this or other OSCAL instances." That request was accepted and Task now includes a uuid field.
  3. Somebody wrote a user story saying "I need a subtask relationship allowing me to say that task 1 includes tasks 1.1, 1.2 and 1.3" Please add a tasks field to the Task assembly. That request was accepted and Task now includes a tasks field.

This is an anti-pattern because taken together those two requests are inconsistent. The uuid field allows tasks to be referenced in this or other OSCAL instances, but the Task/tasks field is not a list of UUIDs, it is a copy of the entire referenceable Task assemblies.

The second user story should have been satisfied by defining a Task/task-refs field of type UUID, meaning that all tasks are defined and can be found at the AP level, not by searching for a specific UUID through a hierarchy of subtasks. And if a Task wants to reuse a Task that has already been created, it cannot do so without copying the whole task, defeating the purpose of making Task referenceable. This is an anti-pattern that should not be followed when creating new self-referencing assemblies, but changing Task/tasks to Task/task-refs (as well as the same self-references in Assessment-Part, Part, Control, and Group) is a breaking change that would need to be deferred until an OSCAL v2 is defined.

@iMichaela
Copy link
Contributor

@davaya we never push new work to main branch or develop branch without discussing the proposed changes with the community under a feature-* branch. The community needs to provide their perspective as well to what is considered 'anti-pattern' or bad practice.

@iMichaela iMichaela self-requested a review July 10, 2024 15:30
Copy link
Contributor

@iMichaela iMichaela left a comment

Choose a reason for hiding this comment

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

Please follow the guidance in the comment left above in this PR.

@davaya
Copy link
Author

davaya commented Jul 10, 2024

Thanks - out of 21 branches there is one feature-* (simple-network). It propose a starter system diagram but no OSCAL content that would characterize or assess that system. A "hello world" system example sounds like a great idea that anyone could relate to; the idea of best practices (and anti-practices) for designing the OSCAL layer models themselves (as opposed to OSCAL files) seems like a more limited audience. But I will replace this with a PR against a "feature-oscal-modeling" branch if a maintainer creates one.

@david-waltermire
Copy link
Contributor

Although I don't agree with the premise of this PR, I don't see why a feature branch is needed in this or similar cases, since community members can review a PR based on a personal branch just as easily as a PR based on a feature branch. Both allow community members to comment or suggest changes in the same way. Using personal forks in this way is a widely accepted practice in multi-organizational open source development. Requiring a feature branch to be made first is just more work and required coordination with no real value in collaboration or review. It will result in less contributions due to the extra work involved. This would not be a good outcome. As a community we should be working to reduce inertia to maximize community contributions and review to the greatest extent possible.

What extra value is a feature branch providing here?

@iMichaela
Copy link
Contributor

iMichaela commented Jul 10, 2024

For all - guidance has been published in the OSCAL repo for all related OSCAL repositories: https://github.com/usnistgov/OSCAL/wiki/Contributing-to-OSCAL--development-and-maintenance#branching-for-contributors. The decision was made by the team long ago and I would greatly appreciate if community members respect it. I do not need to elaborate here why the team decided to enforce it, but removing work approved and merged to develop was a nightmare for some of our team members of that time. I will create the branch for @davaya .

I would like to suggest also, since this issue of generating "anti-pattern" or "negative but valid" examples will be very controversial one (based on the extended dialog in OSCAL Lobby but not captured here), to also create an associated issue or a discussion.

@iMichaela
Copy link
Contributor

The discussion #273 was initiated.

@iMichaela
Copy link
Contributor

@davaya
Copy link
Author

davaya commented Jul 11, 2024

Closing this, migrating to feature branch.

@davaya davaya closed this Jul 11, 2024
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.

3 participants