Skip to content

Commit

Permalink
Fix directory-package-mismatch issue when lint called with "." (#1053)
Browse files Browse the repository at this point in the history
Since we need path components to determine the directory, use an
absolute path in the policy rather than a relative one.

Signed-off-by: Anders Eknert <[email protected]>
  • Loading branch information
anderseknert authored Sep 4, 2024
1 parent 66a3a76 commit 7a4811b
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 47 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,6 @@ import rego.v1
import data.regal.config
import data.regal.result

# - METADATA
# description: |
# emit warning notice when package has more parts than the directory,
# # as this should likely **not** fail
# notices contains _notice(message, "warning") if {
# count(_file_path_values) > 0
# count(_file_path_values) < count(_pkg_path_values)

# message := sprintf(
# "package '%s' has more parts than provided directory path '%s'",
# [concat(".", _pkg_path_values), concat("/", _file_path_values)],
# )
# }

report contains violation if {
# get the last n components from file path, where n == count(_pkg_path_values)
file_path_length_matched := array.slice(
Expand Down Expand Up @@ -53,7 +39,7 @@ _pkg_path_values := without_test_suffix if {
}

_file_path_values := array.slice(parts, 0, count(parts) - 1) if {
parts := split(input.regal.file.name, input.regal.environment.path_separator)
parts := split(input.regal.file.abs, input.regal.environment.path_separator)
}

# when a directory path, like `bar/baz`, is shorter than the package
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,21 @@ import data.regal.rules.idiomatic["directory-package-mismatch"] as rule

test_success_directory_structure_package_path_match if {
module := regal.parse_module("foo/bar/baz/p.rego", "package foo.bar.baz")
r := rule.report with input as module with config.for_rule as default_config
r := rule.report with input as module with config.for_rule as _default_config

r == set()
}

test_fail_directory_structure_package_path_mismatch if {
module := regal.parse_module("foo/bar/baz/p.rego", "package foo.baz.bar")
r := rule.report with input as module with config.for_rule as default_config
r := rule.report with input as module with config.for_rule as _default_config

r == with_location({"col": 9, "file": "foo/bar/baz/p.rego", "row": 1, "text": "package foo.baz.bar"})
}

test_success_directory_structure_package_path_match_longer_directory_path if {
module := regal.parse_module("system/directories/foo/bar/baz/p.rego", "package foo.bar.baz")
r := rule.report with input as module with config.for_rule as default_config

r == set()
}

# note that this is not considered a violation but a _notice_ of severity warning
# see corresponding test below
test_success_directory_structure_package_path_match_shorter_directory_path if {
module := regal.parse_module("bar/baz/p.rego", "package foo.bar.baz")
r := rule.report with input as module with config.for_rule as default_config
r := rule.report with input as module with config.for_rule as _default_config

r == set()
}
Expand All @@ -48,7 +39,7 @@ with_location(location) := {{
"title": "directory-package-mismatch",
}}

default_config := {
_default_config := {
"level": "error",
"exclude-test-suffix": true,
}
2 changes: 1 addition & 1 deletion e2e/cli_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ func TestLintPprof(t *testing.T) {
cwd := testutil.Must(os.Getwd())(t)

t.Cleanup(func() {
os.Remove(pprofFile)
_ = os.Remove(pprofFile)
})

err := regal(&stdout, &stderr)("lint", "--pprof", "clock", cwd+filepath.FromSlash("/testdata/violations"))
Expand Down
3 changes: 3 additions & 0 deletions internal/embeds/schemas/regal-ast.json
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,9 @@
},
"file": {
"properties": {
"abs": {
"type": "string"
},
"name": {
"type": "string"
},
Expand Down
4 changes: 4 additions & 0 deletions internal/parse/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package parse
import (
"fmt"
"os"
"path/filepath"
"strings"

"github.com/open-policy-agent/opa/ast"
Expand Down Expand Up @@ -42,10 +43,13 @@ func PrepareAST(name string, content string, module *ast.Module) (map[string]any
return nil, fmt.Errorf("JSON rountrip failed for module: %w", err)
}

abs, _ := filepath.Abs(name)

preparedAST["regal"] = map[string]any{
"file": map[string]any{
"name": name,
"lines": strings.Split(strings.ReplaceAll(content, "\r\n", "\n"), "\n"),
"abs": abs,
},
"environment": map[string]any{
"path_separator": string(os.PathSeparator),
Expand Down
6 changes: 3 additions & 3 deletions pkg/fixer/fixer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func TestFixer(t *testing.T) {
t.Parallel()

policies := map[string][]byte{
"main.rego": []byte(`package test
"test/main.rego": []byte(`package test
allow {
true #no space
Expand Down Expand Up @@ -49,10 +49,10 @@ deny = true

expectedFileFixedViolations := map[string][]string{
// use-assignment-operator is not expected since use-rego-v1 also addresses this in this example
"main.rego": {"no-whitespace-comment", "use-rego-v1"},
"test/main.rego": {"no-whitespace-comment", "use-rego-v1"},
}
expectedFileContents := map[string][]byte{
"main.rego": []byte(`package test
"test/main.rego": []byte(`package test
import rego.v1
Expand Down
30 changes: 15 additions & 15 deletions pkg/linter/linter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
func TestLintWithDefaultBundle(t *testing.T) {
t.Parallel()

input := test.InputPolicy("p.rego", `package p
input := test.InputPolicy("p/p.rego", `package p
import rego.v1
Expand Down Expand Up @@ -76,7 +76,7 @@ camelCase if {
func TestLintWithUserConfig(t *testing.T) {
t.Parallel()

input := test.InputPolicy("p.rego", `package p
input := test.InputPolicy("p/p.rego", `package p
import rego.v1
Expand Down Expand Up @@ -126,7 +126,7 @@ or := 1
}{
{
name: "baseline",
filename: "p.rego",
filename: "p/p.rego",
expViolations: []string{"opa-fmt", "top-level-iteration", "rule-shadows-builtin"},
},
{
Expand All @@ -135,7 +135,7 @@ or := 1
"bugs": {"rule-shadows-builtin": config.Rule{Level: "ignore"}},
"style": {"opa-fmt": config.Rule{Level: "ignore"}},
}},
filename: "p.rego",
filename: "p/p.rego",
expViolations: []string{"top-level-iteration"},
},
{
Expand Down Expand Up @@ -165,7 +165,7 @@ or := 1
"bugs": {"rule-shadows-builtin": config.Rule{Level: "ignore"}},
},
},
filename: "p.rego",
filename: "p/p.rego",
expViolations: []string{"top-level-iteration"},
},
{
Expand All @@ -181,7 +181,7 @@ or := 1
"bugs": {"rule-shadows-builtin": config.Rule{Level: "ignore"}},
},
},
filename: "p.rego",
filename: "p/p.rego",
expViolations: []string{"top-level-iteration"},
},
{
Expand All @@ -197,7 +197,7 @@ or := 1
},
Rules: map[string]config.Category{},
},
filename: "p.rego",
filename: "p/p.rego",
expViolations: []string{"opa-fmt", "top-level-iteration", "rule-shadows-builtin"},
expLevels: []string{"error", "warning", "warning"},
},
Expand All @@ -207,17 +207,17 @@ or := 1
"bugs": {"rule-shadows-builtin": config.Rule{
Level: "error",
Ignore: &config.Ignore{
Files: []string{"p.rego"},
Files: []string{"p/p.rego"},
},
}},
"style": {"opa-fmt": config.Rule{
Level: "error",
Ignore: &config.Ignore{
Files: []string{"p.rego"},
Files: []string{"p/p.rego"},
},
}},
}},
filename: "p.rego",
filename: "p/p.rego",
expViolations: []string{"top-level-iteration"},
},
{
Expand Down Expand Up @@ -312,7 +312,7 @@ or := 1
func TestLintWithGoRule(t *testing.T) {
t.Parallel()

input := test.InputPolicy("p.rego", `package p
input := test.InputPolicy("p/p.rego", `package p
import rego.v1
x := true
Expand Down Expand Up @@ -340,7 +340,7 @@ func TestLintWithUserConfigGoRuleIgnore(t *testing.T) {
"style": {"opa-fmt": config.Rule{Level: "ignore"}},
}}

input := test.InputPolicy("p.rego", `package p
input := test.InputPolicy("p/p.rego", `package p
import rego.v1
x := true
Expand All @@ -360,7 +360,7 @@ func TestLintWithUserConfigGoRuleIgnore(t *testing.T) {
func TestLintWithCustomRule(t *testing.T) {
t.Parallel()

input := test.InputPolicy("p.rego", "package p\n\nimport rego.v1\n")
input := test.InputPolicy("p/p.rego", "package p\n\nimport rego.v1\n")

linter := NewLinter().
WithCustomRules([]string{filepath.Join("testdata", "custom.rego")}).
Expand All @@ -383,7 +383,7 @@ var testLintWithCustomEmbeddedRulesFS embed.FS
func TestLintWithCustomEmbeddedRules(t *testing.T) {
t.Parallel()

input := test.InputPolicy("p.rego", "package p\n\nimport rego.v1\n")
input := test.InputPolicy("p/p.rego", "package p\n\nimport rego.v1\n")

linter := NewLinter().
WithCustomRulesFromFS(testLintWithCustomEmbeddedRulesFS, "testdata").
Expand All @@ -403,7 +403,7 @@ func TestLintWithCustomEmbeddedRules(t *testing.T) {
func TestLintWithCustomRuleAndCustomConfig(t *testing.T) {
t.Parallel()

input := test.InputPolicy("p.rego", "package p\n\nimport rego.v1\n")
input := test.InputPolicy("p/p.rego", "package p\n\nimport rego.v1\n")

userConfig := config.Config{Rules: map[string]config.Category{
"naming": {"acme-corp-package": config.Rule{Level: "ignore"}},
Expand Down

0 comments on commit 7a4811b

Please sign in to comment.