Merge pull request #1701 from gruntwork-io/chore/expanding-linting-to-opa

chore: Expanding linting to `opa`

Author: yhakbar

Makefile

@@ -10,6 +10,6 @@ lint:
 	golangci-lint run ./...
 
 lint-allow-list:
-	golangci-lint run ./modules/random/... ./modules/testing/... ./modules/slack/... ./modules/collections/... ./modules/environment/... ./modules/retry/... ./modules/shell/... ./modules/git/... ./modules/files/... ./modules/oci/... ./modules/version-checker/... ./modules/database/... ./modules/logger/...
+	golangci-lint run ./modules/random/... ./modules/testing/... ./modules/slack/... ./modules/collections/... ./modules/environment/... ./modules/retry/... ./modules/shell/... ./modules/git/... ./modules/files/... ./modules/oci/... ./modules/version-checker/... ./modules/database/... ./modules/logger/... ./modules/opa/...
 
 .PHONY: lint update-lint-config

modules/opa/download_policy.go

@@ -2,6 +2,7 @@ package opa
 
 import (
 	"context"
+	"fmt"
 	"os"
 	"path/filepath"
 	"sync"
@@ -33,7 +34,7 @@ var (
 func DownloadPolicyE(t testing.TestingT, rulePath string) (string, error) {
 	cwd, err := os.Getwd()
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("getting current working directory: %w", err)
 	}
 
 	// File getters are assumed to be a local path reference, so pass through the original path.
@@ -50,6 +51,7 @@ func DownloadPolicyE(t testing.TestingT, rulePath string) (string, error) {
 
 	// First, check if we had already downloaded the source and it is in our cache.
 	baseDir, subDir := getter.SourceDirSubdir(rulePath)
+
 	downloadPath, hasDownloaded := policyDirCache.Load(baseDir)
 	if hasDownloaded {
 		logger.Default.Logf(t, "Previously downloaded %s: returning cached path", baseDir)
@@ -59,16 +61,20 @@ func DownloadPolicyE(t testing.TestingT, rulePath string) (string, error) {
 	// Not downloaded, so use go-getter to download the remote source to a temp dir.
 	tempDir, err := os.MkdirTemp("", "terratest-opa-policy-*")
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("creating temp directory for policy download: %w", err)
 	}
+
 	// go-getter doesn't work if you give it a directory that already exists, so we add an additional path in the
 	// tempDir to make sure we feed a directory that doesn't exist yet.
 	tempDir = filepath.Join(tempDir, "getter")
 
 	logger.Default.Logf(t, "Downloading %s to temp dir %s", rulePath, tempDir)
+
 	if _, err := getter.GetAny(context.Background(), tempDir, baseDir); err != nil {
-		return "", err
+		return "", fmt.Errorf("downloading policy from %s: %w", baseDir, err)
 	}
+
 	policyDirCache.Store(baseDir, tempDir)
+
 	return filepath.Join(tempDir, subDir), nil
 }

modules/opa/download_policy_test.go

@@ -1,7 +1,6 @@
-package opa
+package opa_test
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -12,40 +11,45 @@ import (
 
 	"github.com/gruntwork-io/terratest/modules/files"
 	"github.com/gruntwork-io/terratest/modules/git"
+	"github.com/gruntwork-io/terratest/modules/opa"
 )
 
-// Test to make sure the DownloadPolicyE function returns a local path without processing it.
+// TestDownloadPolicyReturnsLocalPath makes sure the DownloadPolicyE function returns a local path without processing it.
 func TestDownloadPolicyReturnsLocalPath(t *testing.T) {
 	t.Parallel()
 
 	localPath := "../../examples/terraform-opa-example/policy/enforce_source.rego"
-	path, err := DownloadPolicyE(t, localPath)
+	path, err := opa.DownloadPolicyE(t, localPath)
 	require.NoError(t, err)
 	assert.Equal(t, localPath, path)
 }
 
-// Test to make sure the DownloadPolicyE function returns a remote path to a temporary directory.
+// TestDownloadPolicyDownloadsRemote makes sure the DownloadPolicyE function returns a remote path to a temporary
+// directory.
 func TestDownloadPolicyDownloadsRemote(t *testing.T) {
 	t.Parallel()
 
-	curRef := git.GetCurrentGitRef(t)
-	baseDir := fmt.Sprintf("git::https://github.com/gruntwork-io/terratest.git?ref=%s", curRef)
+	curRef := git.GetCurrentGitRefContext(t, t.Context(), "")
+	baseDir := "git::https://github.com/gruntwork-io/terratest.git?ref=" + curRef
 	localPath := "../../examples/terraform-opa-example/policy/enforce_source.rego"
-	remotePath := fmt.Sprintf("git::https://github.com/gruntwork-io/terratest.git//examples/terraform-opa-example/policy/enforce_source.rego?ref=%s", curRef)
+	remotePath := "git::https://github.com/gruntwork-io/terratest.git//examples/terraform-opa-example/policy/enforce_source.rego?ref=" + curRef
 
 	// Make sure we clean up the downloaded file, while simultaneously asserting that the download dir was stored in the
 	// cache.
 	defer func() {
-		downloadPathRaw, inCache := policyDirCache.Load(baseDir)
+		downloadPathRaw, inCache := opa.PolicyDirCache.Load(baseDir)
 		require.True(t, inCache)
+
 		downloadPath := downloadPathRaw.(string)
+
 		if strings.HasSuffix(downloadPath, "/getter") {
 			downloadPath = filepath.Dir(downloadPath)
 		}
+
 		assert.NoError(t, os.RemoveAll(downloadPath))
 	}()
 
-	path, err := DownloadPolicyE(t, remotePath)
+	path, err := opa.DownloadPolicyE(t, remotePath)
 	require.NoError(t, err)
 
 	absPath, err := filepath.Abs(localPath)
@@ -54,12 +58,14 @@ func TestDownloadPolicyDownloadsRemote(t *testing.T) {
 
 	localContents, err := os.ReadFile(localPath)
 	require.NoError(t, err)
+
 	remoteContents, err := os.ReadFile(path)
 	require.NoError(t, err)
 	assert.Equal(t, localContents, remoteContents)
 }
 
-// Test to make sure the DownloadPolicyE function uses the cache if it has already downloaded an existing base path.
+// TestDownloadPolicyReusesCachedDir makes sure the DownloadPolicyE function uses the cache if it has already downloaded
+// an existing base path.
 func TestDownloadPolicyReusesCachedDir(t *testing.T) {
 	t.Parallel()
 
@@ -70,31 +76,34 @@ func TestDownloadPolicyReusesCachedDir(t *testing.T) {
 	// Make sure we clean up the downloaded file, while simultaneously asserting that the download dir was stored in the
 	// cache.
 	defer func() {
-		downloadPathRaw, inCache := policyDirCache.Load(baseDir)
+		downloadPathRaw, inCache := opa.PolicyDirCache.Load(baseDir)
 		require.True(t, inCache)
+
 		downloadPath := downloadPathRaw.(string)
 
 		if strings.HasSuffix(downloadPath, "/getter") {
 			downloadPath = filepath.Dir(downloadPath)
 		}
+
 		assert.NoError(t, os.RemoveAll(downloadPath))
 	}()
 
-	path, err := DownloadPolicyE(t, remotePath)
+	path, err := opa.DownloadPolicyE(t, remotePath)
 	require.NoError(t, err)
 	files.FileExists(path)
 
-	downloadPathRaw, inCache := policyDirCache.Load(baseDir)
+	downloadPathRaw, inCache := opa.PolicyDirCache.Load(baseDir)
 	require.True(t, inCache)
+
 	downloadPath := downloadPathRaw.(string)
 
 	// make sure the second call is exactly equal to the first call
-	newPath, err := DownloadPolicyE(t, remotePath)
+	newPath, err := opa.DownloadPolicyE(t, remotePath)
 	require.NoError(t, err)
 	assert.Equal(t, path, newPath)
 
 	// Also make sure the cache is reused for alternative sub dirs.
-	newAltPath, err := DownloadPolicyE(t, remotePathAltSubPath)
+	newAltPath, err := opa.DownloadPolicyE(t, remotePathAltSubPath)
 	require.NoError(t, err)
 	assert.True(t, strings.HasPrefix(path, downloadPath))
 	assert.True(t, strings.HasPrefix(newAltPath, downloadPath))

modules/opa/eval.go

@@ -1,6 +1,9 @@
+// Package opa provides helpers for running Open Policy Agent (OPA) evaluations in automated tests.
 package opa
 
 import (
+	"context"
+	"fmt"
 	"path/filepath"
 	"strings"
 	"sync"
@@ -15,9 +18,6 @@ import (
 // EvalOptions defines options that can be passed to the 'opa eval' command for checking policies on arbitrary JSON data
 // via OPA.
 type EvalOptions struct {
-	// Whether OPA should run checks with failure.
-	FailMode FailMode
-
 	// Path to rego file containing the OPA rules. Can also be a remote path defined in go-getter syntax. Refer to
 	// https://github.com/hashicorp/go-getter#url-format for supported options.
 	RulePath string
@@ -31,6 +31,9 @@ type EvalOptions struct {
 	// Example: []string{"--strict"} to enable strict mode for the eval subcommand.
 	ExtraArgs []string
 
+	// Whether OPA should run checks with failure.
+	FailMode FailMode
+
 	// The following options can be used to change the behavior of the related functions for debuggability.
 
 	// When true, keep any temp files and folders that are created for the purpose of running opa eval.
@@ -52,7 +55,7 @@ const (
 	NoFail
 )
 
-// EvalE runs `opa eval` on the given JSON files using the configured policy file and result query. Translates to:
+// Eval runs `opa eval` on the given JSON files using the configured policy file and result query. Translates to:
 //
 //	opa eval -i $JSONFile -d $RulePath $ResultQuery
 //
@@ -62,7 +65,8 @@ func Eval(t testing.TestingT, options *EvalOptions, jsonFilePaths []string, resu
 	require.NoError(t, EvalE(t, options, jsonFilePaths, resultQuery))
 }
 
-// EvalE runs `opa eval` on the given JSON files using the configured policy file and result query. Translates to:
+// EvalWithOutput runs `opa eval` on the given JSON files using the configured policy file and result query.
+// Translates to:
 //
 //	opa eval -i $JSONFile -d $RulePath $ResultQuery
 //
@@ -72,6 +76,7 @@ func Eval(t testing.TestingT, options *EvalOptions, jsonFilePaths []string, resu
 func EvalWithOutput(t testing.TestingT, options *EvalOptions, jsonFilePaths []string, resultQuery string) (outputs []string) {
 	outputs, err := EvalWithOutputE(t, options, jsonFilePaths, resultQuery)
 	require.NoError(t, err)
+
 	return
 }
 
@@ -82,6 +87,7 @@ func EvalWithOutput(t testing.TestingT, options *EvalOptions, jsonFilePaths []st
 // This will asynchronously run OPA on each file concurrently using goroutines.
 func EvalE(t testing.TestingT, options *EvalOptions, jsonFilePaths []string, resultQuery string) (err error) {
 	_, err = evalE(t, options, jsonFilePaths, resultQuery)
+
 	return
 }
 
@@ -98,14 +104,17 @@ func EvalWithOutputE(t testing.TestingT, options *EvalOptions, jsonFilePaths []s
 func evalE(t testing.TestingT, options *EvalOptions, jsonFilePaths []string, resultQuery string) (outputs []string, err error) {
 	downloadedPolicyPath, err := DownloadPolicyE(t, options.RulePath)
 	if err != nil {
-		return
+		return nil, fmt.Errorf("downloading policy %s: %w", options.RulePath, err)
 	}
 
 	outputs = make([]string, len(jsonFilePaths))
 	wg := new(sync.WaitGroup)
 	wg.Add(len(jsonFilePaths))
+
 	errorsOccurred := new(multierror.Error)
+
 	errChans := make([]chan error, len(jsonFilePaths))
+
 	for i, jsonFilePath := range jsonFilePaths {
 		errChan := make(chan error, 1)
 		errChans[i] = errChan
@@ -114,13 +123,16 @@ func evalE(t testing.TestingT, options *EvalOptions, jsonFilePaths []string, res
 			outputs[i] = asyncEval(t, wg, errChan, options, downloadedPolicyPath, jsonFilePath, resultQuery)
 		}(i, jsonFilePath)
 	}
+
 	wg.Wait()
+
 	for _, errChan := range errChans {
 		err := <-errChan
 		if err != nil {
 			errorsOccurred = multierror.Append(errorsOccurred, err)
 		}
 	}
+
 	return outputs, errorsOccurred.ErrorOrNil()
 }
 
@@ -135,27 +147,33 @@ func asyncEval(
 	resultQuery string,
 ) (output string) {
 	defer wg.Done()
-	cmd := shell.Command{
+
+	cmd := &shell.Command{
 		Command: "opa",
 		Args:    formatOPAEvalArgs(options, downloadedPolicyPath, jsonFilePath, resultQuery),
 
 		// Do not log output from shell package so we can log the full json without breaking it up. This is ok, because
 		// opa eval is typically very quick.
 		Logger: logger.Discard,
 	}
+
 	output, err := runCommandWithFullLoggingE(t, options.Logger, cmd)
+
 	ruleBasePath := filepath.Base(downloadedPolicyPath)
+
 	if err == nil {
 		options.Logger.Logf(t, "opa eval passed on file %s (policy %s; query %s)", jsonFilePath, ruleBasePath, resultQuery)
 	} else {
 		options.Logger.Logf(t, "Failed opa eval on file %s (policy %s; query %s)", jsonFilePath, ruleBasePath, resultQuery)
-		if options.DebugDisableQueryDataOnError == false {
+
+		if !options.DebugDisableQueryDataOnError {
 			options.Logger.Logf(t, "DEBUG: rerunning opa eval to query for full data.")
 			cmd.Args = formatOPAEvalArgs(options, downloadedPolicyPath, jsonFilePath, "data")
 			// We deliberately ignore the error here as we want to only return the original error.
 			output, _ = runCommandWithFullLoggingE(t, options.Logger, cmd)
 		}
 	}
+
 	errChan <- err
 
 	return
@@ -179,6 +197,8 @@ func formatOPAEvalArgs(options *EvalOptions, rulePath, jsonFilePath, resultQuery
 		args = append(args, "--fail")
 	case FailDefined:
 		args = append(args, "--fail-defined")
+	case NoFail:
+		// No additional flags needed.
 	}
 
 	args = append(
@@ -189,14 +209,16 @@ func formatOPAEvalArgs(options *EvalOptions, rulePath, jsonFilePath, resultQuery
 			resultQuery,
 		}...,
 	)
+
 	return args
 }
 
-// runCommandWithFullLogging will log the command output in its entirety with buffering. This avoids breaking up the
+// runCommandWithFullLoggingE will log the command output in its entirety with buffering. This avoids breaking up the
 // logs when commands are run concurrently. This is a private function used in the context of opa only because opa runs
 // very quickly, and the output of opa is hard to parse if it is broken up by interleaved logs.
-func runCommandWithFullLoggingE(t testing.TestingT, logger *logger.Logger, cmd shell.Command) (output string, err error) {
-	output, err = shell.RunCommandAndGetOutputE(t, cmd)
-	logger.Logf(t, "Output of command `%s %s`:\n%s", cmd.Command, strings.Join(cmd.Args, " "), output)
+func runCommandWithFullLoggingE(t testing.TestingT, lgr *logger.Logger, cmd *shell.Command) (output string, err error) {
+	output, err = shell.RunCommandContextAndGetOutputE(t, context.Background(), cmd)
+	lgr.Logf(t, "Output of command `%s %s`:\n%s", cmd.Command, strings.Join(cmd.Args, " "), output)
+
 	return
 }