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

docs: Provide clarity in policy titles #140

Merged
merged 4 commits into from
Feb 8, 2023

Conversation

derekmurawsky
Copy link
Contributor

@derekmurawsky derekmurawsky commented Feb 6, 2023

This commit adopts the following in an attempt to provide clarity in policy titles and adopt some more standard RFC language.

  • Critical/High rules => Must / Must Not
  • Medium/Low => Should / Should Not

Additionally, it updates some of the policy language to be more clear.

Resolves: #139
See Also: RFC-2119
See Also: RFC-8174

What's being changed?

Is this PR related to an existing issue?

Check off the following:

  • This PR follows the CONTRIBUTION.md guidelines
  • I have self-reviewed my changes before submitting the PR

This commit adopts the following in an attempt to provide clarity in
policy titles and adopt some more standard RFC language.

- Critical/High rules => Must
- Medium/Low => Should

Additionally, it updates some of the policy language to be more clear.

Resolves: Legit-Labs#139
See Also: [RFC-2119](https://www.rfc-editor.org/rfc/rfc2119)
See Also: [RFC-8174](https://www.rfc-editor.org/rfc/rfc8174)
@derekmurawsky derekmurawsky requested a review from a team as a code owner February 6, 2023 02:36
@ghost
Copy link

ghost commented Feb 6, 2023

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

@derekmurawsky
Copy link
Contributor Author

I'm not an expert on the Gitlab side, and I still think there's room for improvement with the language, so any feedback welcome. 😄

Copy link
Contributor

@noamd-legit noamd-legit left a comment

Choose a reason for hiding this comment

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

Awesome work 🔥

Two general requests:

  1. Your editor changed the indentation of the files which causes OPA to fail loading the policies, this is why the pr checks fail
  2. I rather use only the "Should" notation since even HIGH severity policies could be ok under some circumstances

policies/github/member.rego Show resolved Hide resolved
policies/github/member.rego Show resolved Hide resolved
policies/github/repository.rego Outdated Show resolved Hide resolved
policies/github/repository.rego Show resolved Hide resolved
policies/gitlab/member.rego Show resolved Hide resolved
policies/gitlab/repository.rego Outdated Show resolved Hide resolved
policies/gitlab/repository.rego Show resolved Hide resolved
Yaml is particular and needs spaces after colons. It can be hard to
track this down when it's in a comment block. It's even more difficult
when you are new to go and don't realize that the docstrings are pulled
from the compiled code and not the source on disk. Whoops...
@noamd-legit
Copy link
Contributor

I resolved the title comments. Once you change the Must's to Should's I'll merge it.

Thank you for putting in the effort @derekmurawsky!

derekmurawsky and others added 2 commits February 7, 2023 16:04
Per the conversation in [this PR](Legit-Labs#140 (comment))
I changed instanced of `must` to `should` as all of this advice is guidance
that may be followed or not at the discretion of the implementation
teams that use this tool.
@noamd-legit noamd-legit merged commit ade77a9 into Legit-Labs:main Feb 8, 2023
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.

Improve policy language consistency
3 participants