updates for review comments

abcxyz · Oct 21, 2022 · af7bc94 · af7bc94
1 parent 48a935e
commit af7bc94
Show file tree

Hide file tree

Showing 13 changed files with 166 additions and 120 deletions.
diff --git a/Dockerfile b/Dockerfile
@@ -26,4 +26,4 @@ COPY $APP /linter
 USER nobody
 
 ENTRYPOINT ["/linter"]
-CMD ["lint"]
+CMD ["lint"]
diff --git a/README.md b/README.md
@@ -24,10 +24,6 @@ The workflow in '.github/workflows/generate-terraform-checksum.yml' can be used
 ## In a GitHub workflow
 
 ```yaml
-env:
-  terraform_version: '1.3.2'
-  terraform_checksum: '${{ secrets.TERRAFORM_CHECKSUM }}'
-
 jobs:
   secure-setup-terraform:
     runs-on: 'ubuntu-latest'
@@ -38,8 +34,8 @@ jobs:
       name: 'secure-setup-terraform'
       uses: 'bradegler/secure-setup-terraform@v0.0.10'
       with:
-        terraform_version: ${{env.terraform_version}}
-        terraform_checksum: ${{env.terraform_checksum}}
+        terraform_version: '1.3.2'
+        terraform_checksum: '${{ secrets.TERRAFORM_CHECKSUM }}'
     ## Use terraform normally
 ```
 

diff --git a/cmd/lint-local-exec/linter/lint.go b/cmd/lint-local-exec/linter/lint.go
@@ -21,7 +21,7 @@ import (
 	"github.com/hashicorp/hcl/v2/hclsyntax"
 
 	"github.com/bradegler/secure-setup-terraform/cmd/lint-local-exec/version"
-	"github.com/bradegler/secure-setup-terraform/lib"
+	"github.com/bradegler/secure-setup-terraform/pkg/lint"
 )
 
 const violationType = "local-exec"
@@ -32,20 +32,20 @@ type TerraformLinter struct{}
 
 // FindViolations inspects a set of bytes that represent hcl from a terraform configuration file
 // looking for calls to the 'local-exec' provider.
-func (tfl *TerraformLinter) FindViolations(content []byte, path string) ([]lib.ViolationInstance, error) {
+func (tfl *TerraformLinter) FindViolations(content []byte, path string) ([]lint.ViolationInstance, error) {
 	tokens, diags := hclsyntax.LexConfig(content, path, hcl.Pos{Byte: 0, Line: 1, Column: 1})
 	if diags.HasErrors() {
 		return nil, fmt.Errorf("error lexing hcl file contents: [%s]", diags.Error())
 	}
-	instances := []lib.ViolationInstance{}
+	instances := []lint.ViolationInstance{}
 	for _, token := range tokens {
 		if token.Type == hclsyntax.TokenQuotedLit && string(token.Bytes) == violationType {
-			instances = append(instances, lib.ViolationInstance{Path: path, Line: token.Range.Start.Line})
+			instances = append(instances, lint.ViolationInstance{Path: path, Line: token.Range.Start.Line})
 		}
 	}
 	return instances, nil
 }
 
-func (tfl *TerraformLinter) GetSelectors() []string   { return selectors }
-func (tfl *TerraformLinter) GetViolationType() string { return violationType }
-func (tfl *TerraformLinter) GetVersion() string       { return version.HumanVersion }
+func (tfl *TerraformLinter) Selectors() []string   { return selectors }
+func (tfl *TerraformLinter) ViolationType() string { return violationType }
+func (tfl *TerraformLinter) Version() string       { return version.HumanVersion }
diff --git a/cmd/lint-local-exec/linter/lint_test.go b/cmd/lint-local-exec/linter/lint_test.go
@@ -15,51 +15,116 @@
 package linter
 
 import (
-	"fmt"
-	"reflect"
 	"testing"
 
-	"github.com/bradegler/secure-setup-terraform/lib"
+	"github.com/bradegler/secure-setup-terraform/pkg/lint"
+	"github.com/google/go-cmp/cmp"
 )
 
 func TestLint_FindViolations(t *testing.T) {
 	t.Parallel()
 
-	withLocalExec := "resource \"google_project_service\" \"run_api\" {  service = \"run.googleapis.com\"  disable_on_destroy = true}resource \"google_cloud_run_service\" \"run_service\" {  name     = var.service_name  location = var.region  template {    spec {      containers {        image = var.service_image      }    }  }  traffic {    percent         = 100    latest_revision = true  }  depends_on = [google_project_service.run_api, null_resource.echo]}resource \"null_resource\" \"echo\" {  provisioner \"local-exec\" {    command = \"echo this is a bad practice\"  }}"
-	withoutLocalExec := "resource \"google_project_service\" \"run_api\" {  service = \"run.googleapis.com\"  disable_on_destroy = true}resource \"google_cloud_run_service\" \"run_service\" {  name     = var.service_name  location = var.region  template {    spec {      containers {        image = var.service_image      }    }  }  traffic {    percent         = 100    latest_revision = true  }  depends_on = [google_project_service.run_api]"
+	withLocalExec := `
+	resource "google_project_service" "run_api" {  
+		service = "run.googleapis.com"  
+		disable_on_destroy = true
+	}
+	resource "google_cloud_run_service" "run_service" {  
+		name     = var.service_name  
+		location = var.region  
+		template {    
+			spec {      
+				containers {        
+					image = var.service_image      
+				}    
+			}  
+		}  
+		traffic {    
+			percent         = 100    
+			latest_revision = true  
+		}  
+		depends_on = [google_project_service.run_api, null_resource.echo]
+	}
+	resource "null_resource" "echo" {  
+		provisioner "local-exec" {    
+			command = "echo this is a bad practice" 
+		}
+	}
+	`
+	withoutLocalExec := `
+	resource "google_project_service" "run_api" {  
+		service = "run.googleapis.com"  
+		disable_on_destroy = true
+	}
+	resource "google_cloud_run_service" "run_service" {  
+		name     = var.service_name  
+		location = var.region  
+		template {    
+			spec {      
+				containers {        
+					image = var.service_image      
+				}    
+			}  
+		}  
+		traffic {    
+			percent         = 100    
+			latest_revision = true  
+		}  
+		depends_on = [google_project_service.run_api]
+	}
+	`
 
 	cases := []struct {
 		name        string
 		filename    string
 		content     string
 		expectCount int
-		expect      []lib.ViolationInstance
+		expect      []lint.ViolationInstance
 		wantError   bool
 	}{
-		{name: "with local exec", filename: "/my/path/to/testfile1", content: withLocalExec, expectCount: 1, expect: []lib.ViolationInstance{
-			{Path: "/my/path/to/testfile1", Line: 1},
-		}, wantError: false},
-		{name: "without local exec", filename: "/my/path/to/testfile2", content: withoutLocalExec, expectCount: 0, expect: nil, wantError: false},
+		{
+			name:        "with local exec",
+			filename:    "/my/path/to/testfile1",
+			content:     withLocalExec,
+			expectCount: 1,
+			expect: []lint.ViolationInstance{
+				{
+					Path: "/my/path/to/testfile1",
+					Line: 23,
+				},
+			},
+			wantError: false,
+		},
+		{
+			name:        "without local exec",
+			filename:    "/my/path/to/testfile2",
+			content:     withoutLocalExec,
+			expectCount: 0,
+			expect:      []lint.ViolationInstance{},
+			wantError:   false,
+		},
 	}
 
 	for _, tc := range cases {
 		tc := tc // IMPORTANT: don't shadow the test case
 
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
-			fmt.Printf("Test: %s\n", tc.name)
 
 			l := TerraformLinter{}
 			results, err := l.FindViolations([]byte(tc.content), tc.filename)
 			if tc.wantError != (err != nil) {
-				t.Fatalf("expected error: %#v, got: %#v - %v", tc.wantError, err != nil, err)
-			}
-			if tc.expectCount != len(results) {
-				t.Fatalf("execpted results: %d, got: %d", tc.expectCount, len(results))
+				t.Errorf("expected error: %#v, got: %#v - %v", tc.wantError, err != nil, err)
 			}
-			if len(tc.expect) != 0 && !reflect.DeepEqual(results, tc.expect) {
-				t.Fatalf("execpted results did not match: %v, got: %v", tc.expect, results)
+			if diff := cmp.Diff(tc.expect, results); diff != "" {
+				t.Errorf("Results (-want,+got):\n%s", diff)
 			}
+			// if tc.expectCount != len(results) {
+			// 	t.Fatalf("execpted results: %d, got: %d", tc.expectCount, len(results))
+			// }
+			// if len(tc.expect) != 0 && !reflect.DeepEqual(results, tc.expect) {
+			// 	t.Fatalf("execpted results did not match: %v, got: %v", tc.expect, results)
+			// }
 		})
 	}
 }
diff --git a/cmd/lint-local-exec/main.go b/cmd/lint-local-exec/main.go
@@ -22,7 +22,7 @@ import (
 	"syscall"
 
 	"github.com/bradegler/secure-setup-terraform/cmd/lint-local-exec/linter"
-	"github.com/bradegler/secure-setup-terraform/lib"
+	"github.com/bradegler/secure-setup-terraform/pkg/lint"
 )
 
 func main() {
@@ -37,5 +37,5 @@ func run() error {
 		syscall.SIGINT, syscall.SIGTERM)
 	defer done()
 
-	return lib.RunLinter(ctx, os.Args[1:], &linter.TerraformLinter{})
+	return lint.RunLinter(ctx, os.Args[1:], &linter.TerraformLinter{})
 }
diff --git a/cmd/lint-setup-terraform/linter/lint.go b/cmd/lint-setup-terraform/linter/lint.go
@@ -21,7 +21,7 @@ import (
 	"strings"
 
 	"github.com/bradegler/secure-setup-terraform/cmd/lint-setup-terraform/version"
-	"github.com/bradegler/secure-setup-terraform/lib"
+	"github.com/bradegler/secure-setup-terraform/pkg/lint"
 	"gopkg.in/yaml.v3"
 )
 
@@ -34,19 +34,19 @@ type GitHubActionLinter struct{}
 // FindViolations inspects a set of bytes that represent a YAML document that defines
 // a GitHub action workflow looking for steps that use the 'hashicorp/setup-terraform'
 // action.
-func (tfl *GitHubActionLinter) FindViolations(content []byte, path string) ([]lib.ViolationInstance, error) {
+func (tfl *GitHubActionLinter) FindViolations(content []byte, path string) ([]lint.ViolationInstance, error) {
 	reader := bytes.NewReader(content)
 	node, err := parseYAML(reader)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse yaml: %w", err)
 	}
 	if node == nil {
-		return []lib.ViolationInstance{}, nil
+		return nil, nil
 	}
 	if node.Kind != yaml.DocumentNode {
 		return nil, fmt.Errorf("expected document node, got %v", node.Kind)
 	}
-	violations := []lib.ViolationInstance{}
+	violations := []lint.ViolationInstance{}
 	// Top-level object map
 	for _, docMap := range node.Content {
 		_ = docMap
@@ -85,7 +85,7 @@ func (tfl *GitHubActionLinter) FindViolations(content []byte, path string) ([]li
 											uses := step.Content[k+1]
 											// Looking for the specific 'hashicorp/setup-terraform' action
 											if strings.HasPrefix(uses.Value, "hashicorp/setup-terraform") {
-												violations = append(violations, lib.ViolationInstance{Path: path, Line: uses.Line})
+												violations = append(violations, lint.ViolationInstance{Path: path, Line: uses.Line})
 											}
 										}
 									}
@@ -101,9 +101,9 @@ func (tfl *GitHubActionLinter) FindViolations(content []byte, path string) ([]li
 	return violations, nil
 }
 
-func (tfl *GitHubActionLinter) GetSelectors() []string   { return selectors }
-func (tfl *GitHubActionLinter) GetViolationType() string { return violationType }
-func (tfl *GitHubActionLinter) GetVersion() string       { return version.HumanVersion }
+func (tfl *GitHubActionLinter) Selectors() []string   { return selectors }
+func (tfl *GitHubActionLinter) ViolationType() string { return violationType }
+func (tfl *GitHubActionLinter) Version() string       { return version.HumanVersion }
 
 // parseYAML parses the given reader as a yaml node.
 func parseYAML(r io.Reader) (*yaml.Node, error) {

diff --git a/cmd/lint-setup-terraform/linter/lint_test.go b/cmd/lint-setup-terraform/linter/lint_test.go
@@ -16,10 +16,10 @@ package linter
 
 import (
 	"fmt"
-	"reflect"
 	"testing"
 
-	"github.com/bradegler/secure-setup-terraform/lib"
+	"github.com/bradegler/secure-setup-terraform/pkg/lint"
+	"github.com/google/go-cmp/cmp"
 )
 
 func TestLint_FindViolations(t *testing.T) {
@@ -93,11 +93,30 @@ jobs:
 		filename    string
 		content     string
 		expectCount int
-		expect      []lib.ViolationInstance
+		expect      []lint.ViolationInstance
 		wantError   bool
 	}{
-		{name: "yaml without setup-terraform action", filename: "/test/myfile1", content: withoutSetupTerraform, expectCount: 0, expect: nil, wantError: false},
-		{name: "yaml with setup-terraform action", filename: "/test/myfile2", content: withSetupTerraform, expectCount: 1, expect: []lib.ViolationInstance{{Path: "/test/myfile2", Line: 31}}, wantError: false},
+		{
+			name:        "yaml without setup-terraform action",
+			filename:    "/test/myfile1",
+			content:     withoutSetupTerraform,
+			expectCount: 0,
+			expect:      []lint.ViolationInstance{},
+			wantError:   false,
+		},
+		{
+			name:        "yaml with setup-terraform action",
+			filename:    "/test/myfile2",
+			content:     withSetupTerraform,
+			expectCount: 1,
+			expect: []lint.ViolationInstance{
+				{
+					Path: "/test/myfile2",
+					Line: 31,
+				},
+			},
+			wantError: false,
+		},
 	}
 
 	for _, tc := range cases {
@@ -110,13 +129,10 @@ jobs:
 			l := GitHubActionLinter{}
 			results, err := l.FindViolations([]byte(tc.content), tc.filename)
 			if tc.wantError != (err != nil) {
-				t.Fatalf("expected error: %#v, got: %#v - %v", tc.wantError, err != nil, err)
+				t.Errorf("expected error: %#v, got: %#v - %v", tc.wantError, err != nil, err)
 			}
-			if tc.expectCount != len(results) {
-				t.Fatalf("execpted results: %d, got: %d", tc.expectCount, len(results))
-			}
-			if len(tc.expect) != 0 && !reflect.DeepEqual(results, tc.expect) {
-				t.Fatalf("execpted results did not match: %v, got: %v", tc.expect, results)
+			if diff := cmp.Diff(tc.expect, results); diff != "" {
+				t.Errorf("Results (-want,+got):\n%s", diff)
 			}
 		})
 	}

diff --git a/cmd/lint-setup-terraform/main.go b/cmd/lint-setup-terraform/main.go
@@ -22,7 +22,7 @@ import (
 	"syscall"
 
 	"github.com/bradegler/secure-setup-terraform/cmd/lint-setup-terraform/linter"
-	"github.com/bradegler/secure-setup-terraform/lib"
+	"github.com/bradegler/secure-setup-terraform/pkg/lint"
 )
 
 func main() {
@@ -37,5 +37,5 @@ func run() error {
 		syscall.SIGINT, syscall.SIGTERM)
 	defer done()
 
-	return lib.RunLinter(ctx, os.Args[1:], &linter.GitHubActionLinter{})
+	return lint.RunLinter(ctx, os.Args[1:], &linter.GitHubActionLinter{})
 }
diff --git a/go.mod b/go.mod
@@ -2,7 +2,10 @@ module github.com/bradegler/secure-setup-terraform
 
 go 1.18
 
-require github.com/hashicorp/hcl/v2 v2.14.1
+require (
+	github.com/google/go-cmp v0.3.1
+	github.com/hashicorp/hcl/v2 v2.14.1
+)
 
 require (
 	github.com/agext/levenshtein v1.2.1 // indirect

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json