Address Rego Linter (Regal) Rules (#640)

* Change ignore to warning for specified rules in issue * update style guide for new linter rules * change to warning for test-outside-test-package and rule-length * Update .regal/config.yaml Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com> * Update .regal/config.yaml Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com> * Update CONTENTSTYLEGUIDE.md Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com> * Update CONTENTSTYLEGUIDE.md Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com> * updated opa rego intro paragraph * removed extra sentance * removed bulletpoint * fix capitalization * add punctuation * word smithing * Update CONTENTSTYLEGUIDE.md Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com> --------- Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>
cisagov · Dec 14, 2023 · 1a0b619 · 1a0b619
1 parent e097b5c
commit 1a0b619
Show file tree

Hide file tree

Showing 2 changed files with 118 additions and 10 deletions.
diff --git a/.regal/config.yaml b/.regal/config.yaml
@@ -10,6 +10,7 @@ rules:
       # none is provided. It's harmless, but should be
       # fixed anyway so real issues aren't missed.
       level: warning
+      level: warning
   custom:
     # https://docs.styra.scom/regal/rules/custom/naming-convention
     naming-convention:
@@ -61,10 +62,13 @@ rules:
       # for when that doesn't work.
       non-breakable-word-threshold: 100
       level: warning
+      non-breakable-word-threshold: 100
+      level: warning
     # https://docs.styra.com/regal/rules/style/no-whitespace-comment
     no-whitespace-comment:
       # This repo is actually good about this, but frequently
       # uses '#--' as a delimiter of sorts. That should be OK,
+      # uses '#--' as a delimiter of sorts. That should be OK,
       # and the next version of Regal will allow for exceptions
       # like this: https://github.com/StyraInc/regal/issues/379
       level: ignore
@@ -74,6 +78,7 @@ rules:
     # https://docs.styra.com/regal/rules/style/prefer-some-in-iteration
     prefer-some-in-iteration:
       level: warning
+      level: warning
     # https://docs.styra.com/regal/rules/style/prefer-snake-case
     prefer-snake-case:
       # This is the default style preference for Rego, but since
@@ -85,7 +90,7 @@ rules:
     rule-length:
       level: warning
       max-rule-length: 30
-      count-comments: false
+      count-comments: false      
     # https://docs.styra.com/regal/rules/style/todo-comment
     todo-comment:
       level: ignore
@@ -94,7 +99,10 @@ rules:
     identically-named-tests:
       # Only a few of these — would be easy to fix
       level: warning
+      # Only a few of these — would be easy to fix
+      level: warning
     # https://docs.styra.com/regal/rules/testing/test-outside-test-package
     test-outside-test-package:
       # This is just a style preference
       level: warning
+      level: warning
diff --git a/CONTENTSTYLEGUIDE.md b/CONTENTSTYLEGUIDE.md
@@ -1,5 +1,6 @@
 # Content style guide for SCuBA <!-- omit in toc -->
 
+Welcome to the content style guide for ScubaGear!
 Welcome to the content style guide for ScubaGear!
 
 These guidelines are specific to style rules for PowerShell and OPA Rego code. For general style questions or guidance on topics not covered here, ask or go with best guess and bring up at a later meeting.
@@ -11,10 +12,12 @@ Use menu icon on the top left corner of this document to get to a specific secti
 - Our style guide aims for simplicity. Guidelines should be easy to apply to a range of scenarios.
 - Decisions aren’t about what’s right or wrong according to the rules, but about what’s best practice and improves readability. We're flexible and open to change while maintaining consistency.
 - When making a style or structure decision, we consider the readability, maintainability and ability for consistency in a range of situations.
+- When making a style or structure decision, we consider the readability, maintainability and ability for consistency in a range of situations.
 - When a question specific to help documentation isn’t covered by the style guide, we think it through using these principles, then make a decision and bring it up in the next meeting for deliberation.
 
 ## OPA Rego
 
+The project is adopting the following public Rego [style guide](https://docs.styra.com/opa/rego-style-guide), except where our guide specifically notes an exception (e.g., variable name case). For consistency, we will be using many of the same style rules as PowerShell. There are also a few best practice rules that this project will follow. These best practices were chosen to enhance readability. We recognize that the code is in a constant state of improvement, so the best practices are subject to change. The project is also integrating the [Regal](https://github.com/StyraInc/regal) linter into its automated checks to promote style guide adherence.
 The project is adopting the following public Rego [style guide](https://docs.styra.com/opa/rego-style-guide), except where our guide specifically notes an exception (e.g., variable name case). For consistency, we will be using many of the same style rules as PowerShell. There are also a few best practice rules that this project will follow. These best practices were chosen to enhance readability. We recognize that the code is in a constant state of improvement, so the best practices are subject to change. The project is also integrating the [Regal](https://github.com/StyraInc/regal) linter into its automated checks to promote style guide adherence.
 
 ### Test Cases
@@ -23,7 +26,9 @@ Test names will use the syntax `test_mainVar_In/correct_*V#` to support brevity
 
 ```
 test_ExampleVar_Correct_V1 if {
-    Output := <Product>.tests with input as {
+    PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
+
+    Output := tests with input as {
         "example_policies" : [
             {
                 "Example3" : "ExampleString",
@@ -40,7 +45,9 @@ test_ExampleVar_Correct_V2 if {
 }
 
 test_ExampleVar_Incorrect if {
-    Output := <Product>.tests with input as {
+    PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
+
+    Output := tests with input as {
         "example_policies" : [
             {
                 "Example3" : "ExampleString",
@@ -55,6 +62,7 @@ test_ExampleVar_Incorrect if {
 
 ### Not Implemented
 
+If the policy is untestable at this time, use the templates below.
 If the policy is untestable at this time, use the templates below.
 
 #### Config
@@ -63,6 +71,13 @@ The first one directs the user to the baseline document for manual checking. The
 
 ```
 # At this time we are unable to test for X because of Y
+tests contains {
+    "PolicyId": "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",
+    "Criticality": "Should/Not-Implemented",
+    "Commandlet": [],
+    "ActualValue": [],
+    "ReportDetails": NotCheckedDetails("MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"),
+    "RequirementMet": false,
 tests contains {
     "PolicyId": "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",
     "Criticality": "Should/Not-Implemented",
@@ -75,6 +90,13 @@ tests contains {
 
 ```
 # At this time we are unable to test for X because of Y
+tests contains {
+    "PolicyId": "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>,
+    "Criticality": "Shall/3rd Party",
+    "Commandlet": [],
+    "ActualValue": [],
+    "ReportDetails": DefenderMirrorDetails("MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"),
+    "RequirementMet": false,
 tests contains {
     "PolicyId": "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>,
     "Criticality": "Shall/3rd Party",
@@ -89,6 +111,7 @@ tests contains {
 ```
 test_NotImplemented_Correct if {
     PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
+    PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
 
     Output := <Product>.tests with input as { }
 
@@ -98,10 +121,15 @@ test_NotImplemented_Correct if {
 ```
 test_3rdParty_Correct_V1 if {
     PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
+    PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
 
     Output := <Product>.tests with input as { }
 
-    IncorrectTestResult(PolicyId, Output, DefenderMirrorDetails(PolicyId)) == true
+    RuleOutput := [Result | Result = Output[_]; Result.PolicyId == PolicyId]
+
+    count(RuleOutput) == 1
+    not RuleOutput[0].RequirementMet
+    RuleOutput[0].ReportDetails == DefenderMirrorDetails(PolicyId)
 }
 ```
 
@@ -119,7 +147,9 @@ One True Brace - requires that every braceable statement should have the opening
 
 ```
 test_Example_Correct if {
-    Output := <Product>.tests with input as {
+    PolicyId := "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>"
+
+    Output := tests with input as {
         "example_tag" : {
             "ExampleVar" : false
         }
@@ -143,7 +173,7 @@ Example contains Example.Id if {
     Example.State == "Enabled"
 }
 
-tests contains {
+tests[{
     "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",
     "Criticality" : "Shall",
     "Commandlet" : "Example-Command",
@@ -155,15 +185,15 @@ tests contains {
     Status := ExampleVar == 15
 }
 
-tests {
+tests[{
     "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
 ...
 ```
 
 2) Two blank lines between subsections
 
 ```
-tests contains {
+tests[{
     "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
     "Criticality" : "Should",
     "Commandlet" : "Example-Command",
@@ -192,13 +222,16 @@ tests contains {
 ###################
 ```
 2) Indicate the beginning of every policy.
+2) Indicate the beginning of every policy.
 
 ```
 #
 # MS.<Product>.<Policy Group #>.<Policy #>v<Version #>
+# MS.<Product>.<Policy Group #>.<Policy #>v<Version #>
 #--
 ```
 
+3) Indicate the end of every policy.
 3) Indicate the end of every policy.
 
 ```
@@ -213,12 +246,13 @@ tests contains {
 
 ### Booleans
 
+In the interest of consistency across policy tests and human readability of the test, boolean-valued variables should be set via a comparison test against a boolean constant (true/false) for variables.
 In the interest of consistency across policy tests and human readability of the test, boolean-valued variables should be set via a comparison test against a boolean constant (true/false) for variables.
 
 #### Correct
 
 ```
-tests contains {
+tests[{
     "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
     "Criticality" : "Should",
     "Commandlet" : "Example-Command",
@@ -230,7 +264,7 @@ tests contains {
     Status := ExampleVar == true
 }
 
-tests contains {
+tests[{
     "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
     "Criticality" : "Should",
     "Commandlet" : "Example-Command",
@@ -251,6 +285,7 @@ tests contains {
 } if {
     ExampleVar := input.ExampleVar
     Status := ExampleVar # Missing == true
+    Status := ExampleVar # Missing == true
 }
 
 tests contains {
@@ -266,6 +301,46 @@ Because methods can return undefined, use `not` instead of false comparison when
 #### Correct
 
 
+```
+ExampleVariable contains SomeVariable.DisplayName if {
+    some X
+    SomeVariable := input.example_key[X]
+    not is_null(SomeVariable.OnPremisesImmutableId)
+}
+```
+
+#### Incorrect
+
+```
+ExampleVariable contains SomeVariable.DisplayName if {
+    some X
+    SomeVariable := input.example_key[X]
+    is_null(SomeVariable.OnPremisesImmutableId) == false
+}
+```
+
+Opa will use implicit true in some cases. For example, if the variable contained a true/false boolean, you did not need `== true` to check if the boolean contained is true. This is also true in method returns. Opa assumes methods return true/false & if not specified, will return true. For readability purposes, we enforce explicitly stating `:= true`.
+
+#### Correct
+```
+ExampleMethod(Variable) := true if {
+    count({x | some x in Variable}) == 0
+}
+```
+
+#### Incorrect
+
+```
+ExampleMethod(Variable) if {
+    count({x | some x in Variable}) == 0
+}
+```
+
+Because methods can return undefined, use `not` instead of false comparison when dealing with booleans. `not` will still pass if the you want the false result, i.e. `== false`, but will also pass if the result is undefined. This is important because the default keyword does not work for methods, only variables. So you write your methods for the true cases & treat false/undefined results as failing cases.
+
+#### Correct
+
+
 ```
 ExampleVariable contains SomeVariable.DisplayName if {
     some X
@@ -303,9 +378,11 @@ ExampleMethod(Variable) if {
 
 ### Taking input
 
+We will always store the input in a variable first thing. It can sometimes be easier to only use `input.ExampleVar` repeatedly, but for consistency this is best practice. the `some var in` is added to the end for when you are anticipating an array, so the program has to loop through the input. There are other ways to take in input, but OPA Documents states `input.VariableName` is recommended. As such we will only use this method for consistency. If there is a problem, it can be taken up on a case by case basis for discussion.
 We will always store the input in a variable first thing. It can sometimes be easier to only use `input.ExampleVar` repeatedly, but for consistency this is best practice. the `some var in` is added to the end for when you are anticipating an array, so the program has to loop through the input. There are other ways to take in input, but OPA Documents states `input.VariableName` is recommended. As such we will only use this method for consistency. If there is a problem, it can be taken up on a case by case basis for discussion.
 
 ```
+tests contains {
 tests contains {
     ...
 } if {
@@ -317,11 +394,23 @@ tests contains {
     ...
 } if {
     some ExampleVar in input.example_var[_].nested_var
+} if {
+    some ExampleVar in input.example_var
     Status := "Example" in ExampleVar
 }
 
+tests contains {
+    ...
+} if {
+    some ExampleVar in input.example_var[_].nested_var
+    Status := "Example" in ExampleVar
+}
+
+tests contains {
 tests contains {
     ...
+} if {
+    ExampleVar := input.example_var
 } if {
     ExampleVar := input.example_var
     Status := ExampleVar == true
@@ -330,17 +419,23 @@ tests contains {
 
 ### ActualValue
 
+It can be tempting to put the status variable in the ActualValue spot when you are anticipating a boolean. DON'T! For consistency and as best practice put `ExampleVar.ExampleSetting`.
 It can be tempting to put the status variable in the ActualValue spot when you are anticipating a boolean. DON'T! For consistency and as best practice put `ExampleVar.ExampleSetting`.
 
+#### Incorrect
 #### Incorrect
 ```
+tests contains {
+    "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
 tests contains {
     "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
     "Criticality" : "Should",
     "Commandlet" : "Example-Command",
     "ActualValue" : Status,
     "ReportDetails" : ReportDetailsBoolean(Status),
     "RequirementMet" : Status
+} if {
+    ExampleVar := input.example_var
 } if {
     ExampleVar := input.example_var
     Status := ExampleVar == true
@@ -349,18 +444,23 @@ tests contains {
 
 #### Correct
 ```
+tests contains {
+    "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
 tests contains {
     "PolicyId" : "MS.<Product>.<Policy Group #>.<Policy #>v<Version #>",,
     "Criticality" : "Should",
     "Commandlet" : "Example-Command",
     "ActualValue" : ExampleVar.ExampleSetting,
     "ReportDetails" : ReportDetailsBoolean(Status),
     "RequirementMet" : Status
+} if {
+    ExampleVar := input.example_var
 } if {
     ExampleVar := input.example_var
     Status := ExampleVar == true
 }
 ```
 
+
 ## PowerShell
 [PoshCode's The PowerShell Best Practices and Style Guide](https://github.com/PoshCode/PowerShellPracticeAndStyle)