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

Re-implement replaced_by functionality from #130 #136

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

evankanderson
Copy link
Contributor

After the merge of the governance part of #130, the second half (creating a mechanism for tracking criteria renaming) was still pending (and the script had been re-arranged).

This PR does the following:

  • Re-introduces the replaced_by field for renumbering / retiring criteria (currently a single rule, but we could make it a list if that makes more sense)
  • A bit of cleanup:
    • Return an errors.Join() of detected errors, rather than log.Printf() each error and then returning a new "errors occurred" error. This may help if we use the code as a library later.
    • Use slices.Contains() rather than writing our own contains method.
    • Sort the criteria in each category by name.

Copy link
Contributor

@SecurityCRob SecurityCRob left a comment

Choose a reason for hiding this comment

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

I approve the idea, but defer to our development experts on the execution

@funnelfiasco
Copy link
Contributor

This doesn't appear to quite work. I added replaced_by: OSPS-XY-01 to an entry and it was not rendered.

But before we try to fix that, I'm thinking about how this would actually be used. If we adopt something like the process I proposed in #116, this field doesn't need to be rendered, it's just a note in the yaml for future reference. We should probably settle on our maintenance process before we start implementing it.

cmd/template.md Outdated Show resolved Hide resolved
Copy link
Contributor

@eddie-knight eddie-knight left a comment

Choose a reason for hiding this comment

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

let's address the functionality comment from @funnelfiasco — I'm happy with the intent here though

evankanderson and others added 2 commits January 17, 2025 11:13
Co-authored-by: Eddie Knight <[email protected]>
Signed-off-by: Evan Anderson <[email protected]>
Signed-off-by: Evan Anderson <[email protected]>
@evankanderson
Copy link
Contributor Author

This doesn't appear to quite work. I added replaced_by: OSPS-XY-01 to an entry and it was not rendered.

I added the following to the criteria in the OSPS-DO.yaml file, and it seemed to work, though the formatting was a bit funky (I've since fixed it). Note that you need to reference a valid other rule, or the build breaks (it checks the references).

  - id: OSPS-DO-01
    replaced_by: OSPS-GV-03

This now renders as:

image

But before we try to fix that, I'm thinking about how this would actually be used. If we adopt something like the process I proposed in #116, this field doesn't need to be rendered, it's just a note in the yaml for future reference. We should probably settle on our maintenance process before we start implementing it.

My thought was that people may also either have links to "latest" or simply search the latest for these strings, so it would be handy to have a 2-line link for folks.

(This PR should now be ready, if we decide we want to be able to track this -- we can always iterate on the template rendering later if desired.)

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.

4 participants