implement PR review suggestions

StyraInc · Aug 29, 2024 · e893548 · e893548
1 parent dbfd68c
commit e893548
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 54 deletions.
diff --git a/README.md b/README.md
@@ -365,28 +365,6 @@ reached. If no configuration file is found, Regal will use the default configura
 A custom configuration may be also be provided using the `--config-file`/`-c` option for `regal lint`, which when
 provided will be used to override the default configuration.
 
-### Loading Capabilities from URLs
-
-Starting with Regal version TODO, Regal can load capabilities from URLs with
-the `file`, `http`, or `https` schemes using the `capabilities.from.url` config
-key. For example, to load capabilities from
-`https://example.org/capabilities.json`, this configuration could be used:
-
-```yaml
-capabilities:
-  from:
-    url: https://example.org/capabilities.json
-```
-
-### Supported Engines
-
-Regal includes capabilities files for the following engines:
-
-| Engine | Website | Description |
-|--------|---------|-------------|
-| `opa`  | [OPA website](https://www.openpolicyagent.org/) | Open Policy Agent |
-| `eopa` | [Enterprise OPA website](https://www.styra.com/enterprise-opa/) | Styra Enterprise OPA |
-
 ## Ignoring Rules
 
 If one of Regal's rules doesn't align with your team's preferences, don't worry! Regal is not meant to be the law,
@@ -588,6 +566,28 @@ capabilities:
           type: object
 ```
 
+### Loading Capabilities from URLs
+
+Starting with Regal version TODO, Regal can load capabilities from URLs with
+the `http`, or `https` schemes using the `capabilities.from.url` config key.
+For example, to load capabilities from `https://example.org/capabilities.json`,
+this configuration could be used:
+
+```yaml
+capabilities:
+  from:
+    url: https://example.org/capabilities.json
+```
+
+### Supported Engines
+
+Regal includes capabilities files for the following engines:
+
+| Engine | Website | Description |
+|--------|---------|-------------|
+| `opa`  | [OPA website](https://www.openpolicyagent.org/) | Open Policy Agent |
+| `eopa` | [Enterprise OPA website](https://www.styra.com/enterprise-opa/) | Styra Enterprise OPA |
+
 ## Exit Codes
 
 Exit codes are used to indicate the result of the `lint` command. The `--fail-level` provided for `regal lint` may be

diff --git a/build/do.rq b/build/do.rq
@@ -162,6 +162,7 @@ test {
 
 e2e {
 	run("go test -tags e2e ./e2e")
+	run("go test -tags integration ./internal/capabilities")
 }
 
 lint {
@@ -236,13 +237,12 @@ fetch_eopa_caps {
 		}
 	)
 
-	{ rq.error(sprintf("\nstdout: %s\nstderr: %s\n", [eopa_tags_result.stdout, eopa_tags_result.stderr])) | eopa_tags_result.exitcode != 0 }
-
+	error_nonzero(eopa_tags_result, "failed to fetch tags from GitHub")
 
 	# We assume that tags and capabilities files are 1:1, but some EOPA
 	# release tags in the past did not correctly get capabilities files, so
 	# we eliminate them from consideration.
-	
+
 	known_bad_tags := {
 		"v1.15.0",   # tag missing capabilities file (misnamed v0.15.0)
 		"v1.4.1",    # tag missing capabilities file
@@ -254,13 +254,26 @@ fetch_eopa_caps {
 	# they really have a need, they can manually download them and use the
 	# file locally.
 
-	eopa_tags := {t | some r in eopa_tags_result.stdout; t := r[2] ; not known_bad_tags[t] ; not startswith(t, "v0.")}
+	eopa_tags := {
+		t
+		|
+		some r in eopa_tags_result.stdout
+		t := r[2]
+		not known_bad_tags[t]
+		not startswith(t, "v0.")
+	}
 
 	# Get a directory listing for the capabilities directory, filtering for
 	# only nonzero size files with JSON extensions. The size check is to
 	# avoid long-tail edge cases where we crashed after opening the file
 	# for writing but before committing any content.
-	eopa_caps_tree := {p: f | f := rq.tree(eopa_caps_dir, {})[p]; f.size != 0; f.ext == "json"}
+	eopa_caps_tree := {
+		p: f
+		|
+		f := rq.tree(eopa_caps_dir, {})[p]
+		f.size != 0
+		f.ext == "json"
+	}
 
 	# Determine which capabilities files are missing, what URL they
 	# should be fetched from, and where they should end up on disk.
@@ -273,10 +286,7 @@ fetch_eopa_caps {
 		t := eopa_tags[_]
 		p := rq.joinpath([eopa_caps_dir, sprintf("%s.json", [t])])
 
-		# eopa_caps_tree has object values, so if the path is missing,
-		# retrieving a non-object type is sufficient to check for
-		# nonexistance
-		object.get(eopa_caps_tree, p, "-") == "-"
+		not eopa_caps_tree[p]
 
 		# construct the URL to fetch the content from
 		r := rq.template("https://raw.githubusercontent.com/StyraInc/enterprise-opa/main/capabilities/{{.tag}}.json", {"tag": t})
@@ -291,13 +301,10 @@ fetch_eopa_caps {
 		m := missing_locally[_]
 		print("\tfetching ", m.remote)
 		resp := http.send({"url": m.remote, "method": "GET"})
-			
+
 		{ rq.error(sprintf("non-200 status code '%d' for URL '%s'", [resp.status_code, m.remote])) | resp.status_code != 200 }
 
 		c := resp.raw_body
-
-		# sanity check in case we got the URL wrong
-		not regex.match("404: Not Found", c)
 	}
 
 	# Commit the retrieved content to disk.
@@ -372,3 +379,10 @@ github(what, j) {
 } else := true
 
 is_github if rq.env().GITHUB_ACTION
+
+error_nonzero(run_result, message) if {
+	run_result.exitcode != 0
+	rq.error(sprintf("%s\nstdout:%s\nstderr:\n%s\n", [message, run_result.stdout, run_result.stderr]))
+} else {
+	true
+}
diff --git a/internal/capabilities/capabilities_integration_test.go b/internal/capabilities/capabilities_integration_test.go
@@ -0,0 +1,30 @@
+//go:build integration
+// +build integration
+
+package capabilities
+
+import (
+	"context"
+	"testing"
+)
+
+// Since this test requires internet access, we hide it behind a flag.
+
+func TestLookupFromURL(t *testing.T) {
+	t.Parallel()
+
+	// Test that we can load a one of the existing OPA capabilities files
+	// via GitHub.
+
+	caps, err := Lookup(
+		context.Background(),
+		"https://raw.githubusercontent.com/open-policy-agent/opa/main/capabilities/v0.55.0.json",
+	)
+	if err != nil {
+		t.Errorf("unexpected error from Lookup: %v", err)
+	}
+
+	if len(caps.Builtins) != 193 {
+		t.Errorf("OPA v0.55.0 capabilities should have 193 builtins, not %d", len(caps.Builtins))
+	}
+}
diff --git a/internal/capabilities/capabilities_test.go b/internal/capabilities/capabilities_test.go
@@ -33,25 +33,6 @@ func TestLookupFromFile(t *testing.T) {
 	}
 }
 
-func TestLookupFromURL(t *testing.T) {
-	t.Parallel()
-
-	// Test that we can load a one of the existing OPA capabilities files
-	// via GitHub.
-
-	caps, err := Lookup(
-		context.Background(),
-		"https://raw.githubusercontent.com/open-policy-agent/opa/main/capabilities/v0.55.0.json",
-	)
-	if err != nil {
-		t.Errorf("unexpected error from Lookup: %v", err)
-	}
-
-	if len(caps.Builtins) != 193 {
-		t.Errorf("OPA v0.55.0 capabilities should have 193 builtins, not %d", len(caps.Builtins))
-	}
-}
-
 func TestLookupFromEmbedded(t *testing.T) {
 	t.Parallel()