From 2963c82fdee43d4e5ee563749943751954f148a1 Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Sun, 6 Jul 2025 11:37:39 +0200 Subject: [PATCH] Use Regal for linting Rego (#7752) Closing the circle here, or something. Not a lot of Rego used in OPA yet, but some in examples and tests. The little there is should be linted though, and it'd be good if any new addition of policies got linted by default. But more than anything, the "ignore configuration" provided here avoids having developers seeing thousands of issues reported by Regal when they open the OPA project in VS Code or their editor of choice. Someone might want to look into un-ignoring the doc directory at some point, as it's probably a good idea to have the docs follow best practices. Signed-off-by: Anders Eknert --- .github/workflows/pull-request.yaml | 25 +++--- .regal/config.yaml | 24 +++++ build/policy/{ => files}/files.rego | 88 ++++++++++--------- build/policy/files/files_test.rego | 25 ++++++ build/policy/files_test.rego | 27 ------ build/policy/helpers.rego | 15 ---- build/policy/schema/files.json | 84 ++++++++++++++++++ .../wasm/sdk/examples/basic/example-1.rego | 2 +- .../wasm/sdk/examples/basic/example-2.rego | 2 +- .../wasm/sdk/examples/loaders/example.rego | 2 +- v1/loader/loader_test.go | 6 +- .../embedtest/bar/bar.rego | 0 .../embedtest/bar/bar.yaml | 0 .../embedtest/baz/qux/qux.json | 0 .../{internal => testdata}/embedtest/foo.json | 0 15 files changed, 200 insertions(+), 100 deletions(-) create mode 100644 .regal/config.yaml rename build/policy/{ => files}/files.rego (55%) create mode 100644 build/policy/files/files_test.rego delete mode 100644 build/policy/files_test.rego delete mode 100644 build/policy/helpers.rego create mode 100644 build/policy/schema/files.json rename v1/loader/{internal => testdata}/embedtest/bar/bar.rego (100%) rename v1/loader/{internal => testdata}/embedtest/bar/bar.yaml (100%) rename v1/loader/{internal => testdata}/embedtest/baz/qux/qux.json (100%) rename v1/loader/{internal => testdata}/embedtest/foo.json (100%) diff --git a/.github/workflows/pull-request.yaml b/.github/workflows/pull-request.yaml index f4877a8f11..c330f65f19 100644 --- a/.github/workflows/pull-request.yaml +++ b/.github/workflows/pull-request.yaml @@ -439,24 +439,29 @@ jobs: version: edge - name: Test policies - run: opa test --v0-compatible build/policy + run: opa test --schema build/policy/schema --bundle build/policy - name: Ensure proper formatting - run: opa fmt --v0-compatible --list --fail build/policy + run: opa fmt --list --fail build/policy - name: Run file policy checks on changed files run: | curl --silent --fail --header 'Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}' -o files.json \ https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}/files - opa eval --v0-compatible -d build/policy/files.rego -d build/policy/helpers.rego --format values --input files.json \ - --fail-defined 'data.files.deny[message]' + opa eval --bundle build/policy --format values --input files.json --fail-defined 'data.files.deny[message]' env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - - name: Show input on failure - run: opa eval --v0-compatible --input files.json --format pretty input - if: ${{ failure() }} + - name: Download Regal + uses: StyraInc/setup-regal@33a142b1189004e0f14bf42b15972c67eecce776 #v1.0.0 + with: + version: latest + + - name: Run Regal lint + # Current configuration ensures anything but build/policy is ignored. While this could point Regal only at that + # directory, this will serve as a reminder when more Rego policies are added, as they should be linted by default. + run: regal lint --format github . docs-build: name: Build Docs @@ -504,18 +509,18 @@ jobs: run: | # Create the input file with all job results echo '${{ toJSON(needs) }}' > input.json - + # Find failed or cancelled jobs using OPA opa eval -d .github/workflows/pull-request.yaml \ --input=input.json \ '{job|some _, job in data.jobs["pr-check-summary"].needs} & {job | input[job].result in {"failure", "cancelled"}}' \ --format=raw > failed_jobs.json - + # Check for failures and display a nice message if [ "$(cat failed_jobs.json)" != "[]" ]; then echo "The following required jobs did not complete successfully:" jq -r '.[]' failed_jobs.json | sed 's/^/- /' exit 1 fi - + echo "All jobs completed successfully or were skipped" diff --git a/.regal/config.yaml b/.regal/config.yaml new file mode 100644 index 0000000000..45da0a4084 --- /dev/null +++ b/.regal/config.yaml @@ -0,0 +1,24 @@ +ignore: + files: + - "docs/*" + - "**/test/*" + - "**/testdata/*" + - "**/testfiles/*" + +rules: + idiomatic: + directory-package-mismatch: + exclude-test-suffix: true + ignore: + files: + - "docs" + - "internal/wasm/sdk/examples/*" + style: + line-length: + ignore: + files: + - "docs" + +project: + roots: + - build/policy diff --git a/build/policy/files.rego b/build/policy/files/files.rego similarity index 55% rename from build/policy/files.rego rename to build/policy/files/files.rego index ed22bd8f2d..16883264c3 100644 --- a/build/policy/files.rego +++ b/build/policy/files/files.rego @@ -1,46 +1,17 @@ -# Expects policy input as provided by: -# https://api.github.com/repos/open-policy-agent/opa/pulls/${PR_ID}/files +# METADATA +# description: | +# Expects policy input as provided by: +# https://api.github.com/repos/open-policy-agent/opa/pulls/${PR_ID}/files # -# Note that the "filename" here refers to the full path of the file, like -# docs/foo/bar.yaml - since that's how it's named in the -# input we'll use the same convention here. - +# Note that the "filename" here refers to the full, relative path of the file, +# like docs/foo/bar.yaml - since that's how it's named in the input we'll use +# the same convention here. +# schemas: +# - input: schema.files package files -import rego.v1 - -import data.helpers.basename -import data.helpers.directory -import data.helpers.extension - -filenames := {f.filename | some f in input} - -changes[filename] := attributes if { - some change in input - filename := change.filename - attributes := object.remove(change, ["filename"]) -} - -http_error(response) if response.status_code == 0 - -http_error(response) if response.status_code >= 400 - -dump_response_on_error(response) := response if { - http_error(response) - print("unexpected error in response", response) -} - -dump_response_on_error(response) := response if not http_error(response) - -get_file_in_pr(filename) := dump_response_on_error(http.send({ - "url": changes[filename].raw_url, - "method": "GET", - "headers": {"Authorization": sprintf("Bearer %v", [opa.runtime().env.GITHUB_TOKEN])}, - "cache": true, - "enable_redirect": true, - "raise_error": false, -})).raw_body - +# METADATA +# entrypoint: true deny contains sprintf("%s is an invalid YAML file: %s", [filename, content]) if { some filename, content in yaml_file_contents changes[filename].status in {"added", "modified"} @@ -53,12 +24,45 @@ deny contains sprintf("%s is an invalid JSON file: %s", [filename, content]) if not json.is_valid(content) } -yaml_file_contents[filename] := get_file_in_pr(filename) if { +yaml_file_contents[filename] := file_in_pr(filename) if { some filename in filenames extension(filename) in {"yml", "yaml"} } -json_file_contents[filename] := get_file_in_pr(filename) if { +json_file_contents[filename] := file_in_pr(filename) if { some filename in filenames extension(filename) == "json" } + +filenames contains f.filename if some f in input + +changes[filename] := attributes if { + some change in input + filename := change.filename + attributes := object.remove(change, ["filename"]) +} + +http_error(response) if response.status_code == 0 +http_error(response) if response.status_code >= 400 + +dump_response_on_error(response) := response if { + http_error(response) + print("unexpected error in response", response) # regal ignore:print-or-trace-call +} + +dump_response_on_error(response) := response if not http_error(response) + +file_in_pr(filename) := dump_response_on_error(http.send({ + "url": changes[filename].raw_url, + "method": "GET", + "headers": {"Authorization": sprintf("Bearer %v", [opa.runtime().env.GITHUB_TOKEN])}, + "cache": true, + "enable_redirect": true, + "raise_error": false, +})).raw_body + +default last_indexof(_, _) := -1 + +last_indexof(string, search) := indices[count(indices) - 1] if indices := indexof_n(string, search) + +extension(filename) := substring(filename, last_indexof(filename, ".") + 1, count(filename) - 1) diff --git a/build/policy/files/files_test.rego b/build/policy/files/files_test.rego new file mode 100644 index 0000000000..009135c1ce --- /dev/null +++ b/build/policy/files/files_test.rego @@ -0,0 +1,25 @@ +package files_test + +import data.files + +test_deny_invalid_yaml_file if { + expected := "invalid.yaml is an invalid YAML file: {null{}}" + expected in files.deny with files.yaml_file_contents as {"invalid.yaml": "{null{}}"} + with files.changes as {"invalid.yaml": {"status": "modified"}} +} + +test_allow_valid_yaml_file if { + count(files.deny) == 0 with files.yaml_file_contents as {"valid.yaml": "foo: bar"} + with files.changes as {"valid.yaml": {"status": "modified"}} +} + +test_deny_invalid_json_file if { + expected := "invalid.json is an invalid JSON file: }}}" + expected in files.deny with files.json_file_contents as {"invalid.json": "}}}"} + with files.changes as {"invalid.json": {"status": "modified"}} +} + +test_allow_valid_json_file if { + count(files.deny) == 0 with files.json_file_contents as {"valid.json": "{\"foo\": \"bar\"}"} + with files.changes as {"valid.json": {"status": "modified"}} +} diff --git a/build/policy/files_test.rego b/build/policy/files_test.rego deleted file mode 100644 index e92b9d1e37..0000000000 --- a/build/policy/files_test.rego +++ /dev/null @@ -1,27 +0,0 @@ -package files_test - -import rego.v1 - -import data.files.deny - -test_deny_invalid_yaml_file if { - expected := "invalid.yaml is an invalid YAML file: {null{}}" - expected in deny with data.files.yaml_file_contents as {"invalid.yaml": "{null{}}"} - with data.files.changes as {"invalid.yaml": {"status": "modified"}} -} - -test_allow_valid_yaml_file if { - count(deny) == 0 with data.files.yaml_file_contents as {"valid.yaml": "foo: bar"} - with data.files.changes as {"valid.yaml": {"status": "modified"}} -} - -test_deny_invalid_json_file if { - expected := "invalid.json is an invalid JSON file: }}}" - expected in deny with data.files.json_file_contents as {"invalid.json": "}}}"} - with data.files.changes as {"invalid.json": {"status": "modified"}} -} - -test_allow_valid_json_file if { - count(deny) == 0 with data.files.json_file_contents as {"valid.json": "{\"foo\": \"bar\"}"} - with data.files.changes as {"valid.json": {"status": "modified"}} -} diff --git a/build/policy/helpers.rego b/build/policy/helpers.rego deleted file mode 100644 index cc302ab8bd..0000000000 --- a/build/policy/helpers.rego +++ /dev/null @@ -1,15 +0,0 @@ -package helpers - -import rego.v1 - -last_indexof(string, search) := i if { - all := [i | chars := split(string, ""); chars[i] == search] - count(all) > 0 - i := all[count(all) - 1] -} else := -1 - -basename(filename) := substring(filename, last_indexof(filename, "/") + 1, count(filename) - 1) - -extension(filename) := substring(filename, last_indexof(filename, ".") + 1, count(filename) - 1) - -directory(filename) := substring(filename, 0, last_indexof(filename, "/")) diff --git a/build/policy/schema/files.json b/build/policy/schema/files.json new file mode 100644 index 0000000000..26102ecdb2 --- /dev/null +++ b/build/policy/schema/files.json @@ -0,0 +1,84 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "files", + "$ref": "#/$defs/files", + "$defs": { + "files": { + "type": "array", + "items": { + "title": "Diff Entry", + "description": "Diff Entry", + "type": "object", + "properties": { + "sha": { + "type": "string", + "example": "bbcd538c8e72b8c175046e27cc8f907076331401" + }, + "filename": { + "type": "string", + "example": "file1.txt" + }, + "status": { + "type": "string", + "enum": [ + "added", + "removed", + "modified", + "renamed", + "copied", + "changed", + "unchanged" + ], + "example": "added" + }, + "additions": { + "type": "integer", + "example": 103 + }, + "deletions": { + "type": "integer", + "example": 21 + }, + "changes": { + "type": "integer", + "example": 124 + }, + "blob_url": { + "type": "string", + "format": "uri", + "example": "https://github.com/octocat/Hello-World/blob/6dcb09b5b57875f334f61aebed695e2e4193db5e/file1.txt" + }, + "raw_url": { + "type": "string", + "format": "uri", + "example": "https://github.com/octocat/Hello-World/raw/6dcb09b5b57875f334f61aebed695e2e4193db5e/file1.txt" + }, + "contents_url": { + "type": "string", + "format": "uri", + "example": "https://api.github.com/repos/octocat/Hello-World/contents/file1.txt?ref=6dcb09b5b57875f334f61aebed695e2e4193db5e" + }, + "patch": { + "type": "string", + "example": "@@ -132,7 +132,7 @@ module Test @@ -1000,7 +1000,7 @@ module Test" + }, + "previous_filename": { + "type": "string", + "example": "file.txt" + } + }, + "required": [ + "additions", + "blob_url", + "changes", + "contents_url", + "deletions", + "filename", + "raw_url", + "sha", + "status" + ] + } + } + } +} diff --git a/internal/wasm/sdk/examples/basic/example-1.rego b/internal/wasm/sdk/examples/basic/example-1.rego index cbc29d0560..cd8c2a63e7 100644 --- a/internal/wasm/sdk/examples/basic/example-1.rego +++ b/internal/wasm/sdk/examples/basic/example-1.rego @@ -1,3 +1,3 @@ package example -allow = input.foo +allow := input.foo diff --git a/internal/wasm/sdk/examples/basic/example-2.rego b/internal/wasm/sdk/examples/basic/example-2.rego index f60b425834..b14ea3b232 100644 --- a/internal/wasm/sdk/examples/basic/example-2.rego +++ b/internal/wasm/sdk/examples/basic/example-2.rego @@ -1,3 +1,3 @@ package example -allow = input.bar +allow := input.bar diff --git a/internal/wasm/sdk/examples/loaders/example.rego b/internal/wasm/sdk/examples/loaders/example.rego index cbc29d0560..cd8c2a63e7 100644 --- a/internal/wasm/sdk/examples/loaders/example.rego +++ b/internal/wasm/sdk/examples/loaders/example.rego @@ -1,3 +1,3 @@ package example -allow = input.foo +allow := input.foo diff --git a/v1/loader/loader_test.go b/v1/loader/loader_test.go index bddb1ab872..ede6491d74 100644 --- a/v1/loader/loader_test.go +++ b/v1/loader/loader_test.go @@ -1011,7 +1011,7 @@ func TestLoadRooted(t *testing.T) { }) } -//go:embed internal/embedtest +//go:embed testdata/embedtest var embedTestFS embed.FS func TestLoadFS(t *testing.T) { @@ -1021,7 +1021,7 @@ func TestLoadFS(t *testing.T) { "three:baz", } - fsys, err := fs.Sub(embedTestFS, "internal/embedtest") + fsys, err := fs.Sub(embedTestFS, "testdata/embedtest") if err != nil { t.Fatalf("Unexpected error: %v", err) } @@ -1060,7 +1060,7 @@ func TestLoadWithJSONOptions(t *testing.T) { "three:baz", } - fsys, err := fs.Sub(embedTestFS, "internal/embedtest") + fsys, err := fs.Sub(embedTestFS, "testdata/embedtest") if err != nil { t.Fatalf("Unexpected error: %v", err) } diff --git a/v1/loader/internal/embedtest/bar/bar.rego b/v1/loader/testdata/embedtest/bar/bar.rego similarity index 100% rename from v1/loader/internal/embedtest/bar/bar.rego rename to v1/loader/testdata/embedtest/bar/bar.rego diff --git a/v1/loader/internal/embedtest/bar/bar.yaml b/v1/loader/testdata/embedtest/bar/bar.yaml similarity index 100% rename from v1/loader/internal/embedtest/bar/bar.yaml rename to v1/loader/testdata/embedtest/bar/bar.yaml diff --git a/v1/loader/internal/embedtest/baz/qux/qux.json b/v1/loader/testdata/embedtest/baz/qux/qux.json similarity index 100% rename from v1/loader/internal/embedtest/baz/qux/qux.json rename to v1/loader/testdata/embedtest/baz/qux/qux.json diff --git a/v1/loader/internal/embedtest/foo.json b/v1/loader/testdata/embedtest/foo.json similarity index 100% rename from v1/loader/internal/embedtest/foo.json rename to v1/loader/testdata/embedtest/foo.json