code review fixes

Author: daniel-cit

0-bootstrap/scripts/choose_build_type.sh

@@ -23,15 +23,7 @@ TARGET_BUILD="$1"
 build_types=("cb" "github" "gitlab" "jenkins" "terraform_cloud")
 
 # Validate the build_type input
-valid_build_type=false
-for valid_type in "${build_types[@]}"; do
-  if [ "$TARGET_BUILD" = "$valid_type" ]; then
-    valid_build_type=true
-    break
-  fi
-done
-
-if [ "$valid_build_type" = false ]; then
+if [[ ! " ${build_types[*]} " =~ " ${TARGET_BUILD} " ]]; then
   echo "Error: Invalid build type '$TARGET_BUILD'.  Must be one of: ${build_types[*]}"
   exit 1
 fi

helpers/foundation-deployer/github/github.go

@@ -19,14 +19,13 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"regexp"
+	"os"
 	"time"
 
 	"github.com/google/go-github/v58/github"
 	"github.com/mitchellh/go-testing-interface"
-	"github.com/terraform-google-modules/terraform-example-foundation/test/integration/testutils"
 
-	localutil "github.com/terraform-google-modules/terraform-example-foundation/helpers/foundation-deployer/utils"
+	"github.com/terraform-google-modules/terraform-example-foundation/helpers/foundation-deployer/utils"
 
 	"golang.org/x/oauth2"
 )
@@ -46,20 +45,6 @@ const (
 	StatusActionRequired = "action_required"
 )
 
-var (
-	retryRegexp = map[*regexp.Regexp]string{}
-)
-
-func init() {
-	for e, m := range testutils.RetryableTransientErrors {
-		r, err := regexp.Compile(fmt.Sprintf("(?s)%s", e)) //(?s) enables dot (.) to match newline.
-		if err != nil {
-			panic(fmt.Sprintf("failed to compile regex %s: %s", e, err.Error()))
-		}
-		retryRegexp[r] = m
-	}
-}
-
 type GH struct {
 	TriggerNewBuild func(t testing.TB, ctx context.Context, owner, repo, token string, runID int64) (int64, string, string, error)
 	sleepTime       time.Duration
@@ -72,6 +57,7 @@ func NewGH() GH {
 		sleepTime:       20,
 	}
 }
+
 // triggerNewBuild triggers a new action execution
 func triggerNewBuild(t testing.TB, ctx context.Context, owner, repo, token string, runID int64) (int64, string, string, error) {
 	ts := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: token})
@@ -80,14 +66,14 @@ func triggerNewBuild(t testing.TB, ctx context.Context, owner, repo, token strin
 
 	resp, err := client.Actions.RerunWorkflowByID(ctx, owner, repo, runID)
 	if err != nil {
-		return 0, "", "", fmt.Errorf("Error re-running workflow: %v", err)
+		return 0, "", "", fmt.Errorf("error re-running workflow: %v", err)
 	}
 	if resp.StatusCode != http.StatusCreated {
 		bodyBytes, err := io.ReadAll(resp.Body)
 		if err != nil {
-			return 0, "", "", fmt.Errorf("Error re-running workflow status: %d body parsing error: %v", resp.StatusCode, err)
+			return 0, "", "", fmt.Errorf("error re-running workflow status: %d body parsing error: %v", resp.StatusCode, err)
 		}
-		return 0, "", "", fmt.Errorf("Error re-running workflow status: %d body: %s", resp.StatusCode, string(bodyBytes))
+		return 0, "", "", fmt.Errorf("error re-running workflow status: %d body: %s", resp.StatusCode, string(bodyBytes))
 	}
 	opts := &github.ListWorkflowRunsOptions{
 
@@ -103,11 +89,11 @@ func triggerNewBuild(t testing.TB, ctx context.Context, owner, repo, token strin
 	runs, _, err := client.Actions.ListRepositoryWorkflowRuns(ctx, owner, repo, opts)
 
 	if err != nil {
-		return 0, "", "", fmt.Errorf("Error listing workflow runs: %v", err)
+		return 0, "", "", fmt.Errorf("error listing workflow runs: %v", err)
 	}
 
 	if len(runs.WorkflowRuns) == 0 {
-		return 0, "", "", fmt.Errorf("No workflow runs found for repo %s/%s ", owner, repo)
+		return 0, "", "", fmt.Errorf("no workflow runs found for repo %s/%s", owner, repo)
 	}
 
 	var newRunID int64
@@ -143,7 +129,7 @@ func (g GH) GetLastActionState(t testing.TB, ctx context.Context, owner, repo, t
 	runs, _, err := client.Actions.ListRepositoryWorkflowRuns(ctx, owner, repo, opts)
 
 	if err != nil {
-		return 0, "", "", fmt.Errorf("Error listing workflow runs: %v", err)
+		return 0, "", "", fmt.Errorf("error listing workflow runs: %v", err)
 	}
 
 	if len(runs.WorkflowRuns) == 0 {
@@ -175,7 +161,7 @@ func (g GH) GetActionState(t testing.TB, ctx context.Context, owner, repo, token
 
 	run, _, err := client.Actions.GetWorkflowRunByID(ctx, owner, repo, runID)
 	if err != nil {
-		return "", "", fmt.Errorf("Error getting workflow run: %v", err)
+		return "", "", fmt.Errorf("error getting workflow run: %v", err)
 	}
 
 	var status string
@@ -201,7 +187,7 @@ func (g GH) GetBuildLogs(t testing.TB, ctx context.Context, owner, repo, token s
 	}
 	jobs, _, err := client.Actions.ListWorkflowJobs(ctx, owner, repo, runID, listJobsOpts)
 	if err != nil {
-		return "", fmt.Errorf("Error listing jobs for run %d: %v", runID, err)
+		return "", fmt.Errorf("error listing jobs for run %d: %v", runID, err)
 	}
 
 	for _, job := range jobs.Jobs {
@@ -214,25 +200,31 @@ func (g GH) GetBuildLogs(t testing.TB, ctx context.Context, owner, repo, token s
 
 			logURL, resp, err := client.Actions.GetWorkflowJobLogs(ctx, owner, repo, jobID, 3)
 			if err != nil {
-				return "", fmt.Errorf("  ERROR: Could not get log URL for job %d: %v\n", jobID, err)
+				return "", fmt.Errorf("error: Could not get log URL for job %d: %v", jobID, err)
 			}
 			if resp.StatusCode != http.StatusFound {
-				return "", fmt.Errorf("  ERROR: Expected a 302 redirect for job logs, but got %s\n", resp.Status)
+				return "", fmt.Errorf("error: Expected a 302 redirect for job logs, but got %s", resp.Status)
 			}
 
 			logContentResp, err := http.Get(logURL.String())
 			if err != nil {
-				return "", fmt.Errorf("  ERROR: Could not download logs from %s: %v\n", logURL, err)
+				return "", fmt.Errorf("error: Could not download logs from %s: %v", logURL, err)
 			}
-			defer logContentResp.Body.Close()
+
+			defer func() {
+				err := logContentResp.Body.Close()
+				if err != nil {
+					fmt.Fprintf(os.Stderr, "error closing execution log file: %s", err)
+				}
+			}()
 
 			if logContentResp.StatusCode != http.StatusOK {
-				return "", fmt.Errorf("  ERROR: Expected status 200 OK from log URL, but got %s\n", logContentResp.Status)
+				return "", fmt.Errorf("error: Expected status 200 OK from log URL, but got %s", logContentResp.Status)
 			}
 
 			bodyBytes, err := io.ReadAll(logContentResp.Body)
 			if err != nil {
-				return "", fmt.Errorf("Error reading response body: %v", err)
+				return "", fmt.Errorf("error reading response body: %v", err)
 			}
 			return string(bodyBytes), nil
 		}
@@ -283,7 +275,7 @@ func (g GH) WaitBuildSuccess(t testing.TB, owner, repo, token string, failureMsg
 	}
 	for i := 0; i < maxErrorRetries; i++ {
 		if status != statusCompleted {
-			status, conclusion, err = g.GetFinalActionState(t, ctx, owner, repo, token, runID, maxBuildRetry)
+			_, conclusion, err = g.GetFinalActionState(t, ctx, owner, repo, token, runID, maxBuildRetry)
 			if err != nil {
 				return err
 			}
@@ -294,7 +286,7 @@ func (g GH) WaitBuildSuccess(t testing.TB, owner, repo, token string, failureMsg
 			if err != nil {
 				return err
 			}
-			if localutil.IsRetryableError(t, logs) {
+			if utils.IsRetryableError(t, logs) {
 				return fmt.Errorf("%s\nSee:\nhttps://github.com/%s/%s/actions/runs/%d\nfor details", failureMsg, owner, repo, runID)
 			}
 			fmt.Println("build failed with retryable error. a new build will be triggered.")

helpers/foundation-deployer/gitlab/gitlab.go

@@ -19,7 +19,6 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"regexp"
 	"time"
 
 	"github.com/mitchellh/go-testing-interface"
@@ -43,10 +42,6 @@ const (
 	StatusCanceling          = "canceling"
 )
 
-var (
-	retryRegexp = map[*regexp.Regexp]string{}
-)
-
 type GL struct {
 	TriggerNewBuild func(t testing.TB, ctx context.Context, owner, project, token string, jobID int) (int, error)
 	sleepTime       time.Duration
@@ -65,12 +60,12 @@ func triggerNewBuild(t testing.TB, ctx context.Context, owner, project, token st
 
 	git, err := gitlab.NewClient(token)
 	if err != nil {
-		return 0, fmt.Errorf("Failed to create client: %v", err)
+		return 0, fmt.Errorf("failed to create client: %v", err)
 	}
 
 	job, resp, err := git.Jobs.RetryJob(fmt.Sprintf("%s/%s", owner, project), jobID, gitlab.WithContext(ctx))
 	if err != nil {
-		return 0, fmt.Errorf("Error retrying job: %v", err)
+		return 0, fmt.Errorf("error retrying job: %v", err)
 	}
 	if resp.StatusCode >= http.StatusBadRequest && resp.StatusCode <= http.StatusNetworkAuthenticationRequired {
 		bodyBytes, err := io.ReadAll(resp.Body)
@@ -86,7 +81,7 @@ func triggerNewBuild(t testing.TB, ctx context.Context, owner, project, token st
 func (g GL) GetLastJobStatus(t testing.TB, ctx context.Context, owner, project, token string) (string, int, error) {
 	git, err := gitlab.NewClient(token)
 	if err != nil {
-		return "", 0, fmt.Errorf("Failed to create client: %v", err)
+		return "", 0, fmt.Errorf("failed to create client: %v", err)
 	}
 
 	includeRetried := true
@@ -102,11 +97,11 @@ func (g GL) GetLastJobStatus(t testing.TB, ctx context.Context, owner, project,
 
 	jobs, _, err := git.Jobs.ListProjectJobs(fmt.Sprintf("%s/%s", owner, project), opts, gitlab.WithContext(ctx))
 	if err != nil {
-		return "", 0, fmt.Errorf("Error listing project jobs: %v", err)
+		return "", 0, fmt.Errorf("error listing project jobs: %v", err)
 	}
 
 	if len(jobs) == 0 {
-		return "", 0, fmt.Errorf("No jobs found for project: %s/%s", owner, project)
+		return "", 0, fmt.Errorf("no jobs found for project: %s/%s", owner, project)
 	}
 
 	return jobs[0].Status, jobs[0].ID, nil
@@ -116,21 +111,21 @@ func (g GL) GetLastJobStatus(t testing.TB, ctx context.Context, owner, project,
 func (g GL) GetJobLogs(t testing.TB, ctx context.Context, owner, project, token string, jobID int) (string, error) {
 	git, err := gitlab.NewClient(token)
 	if err != nil {
-		return "", fmt.Errorf("Failed to create client: %v", err)
+		return "", fmt.Errorf("failed to create client: %v", err)
 	}
 
 	reader, resp, err := git.Jobs.GetTraceFile(fmt.Sprintf("%s/%s", owner, project), jobID)
 	if err != nil {
-		return "", fmt.Errorf("Error getting job trace file: %v", err)
+		return "", fmt.Errorf("error getting job trace file: %v", err)
 	}
 
 	if resp.StatusCode != http.StatusOK {
-		return "", fmt.Errorf("Expected status 200 OK, but got %s for job %d trace", resp.Status, jobID)
+		return "", fmt.Errorf("expected status 200 OK, but got %s for job %d trace", resp.Status, jobID)
 	}
 
 	logBytes, err := io.ReadAll(reader)
 	if err != nil {
-		return "", fmt.Errorf("Failed to read job %d trace from bytes.Reader: %v", jobID, err)
+		return "", fmt.Errorf("failed to read job %d trace from bytes.Reader: %v", jobID, err)
 	}
 
 	return string(logBytes), nil
@@ -140,12 +135,12 @@ func (g GL) GetJobLogs(t testing.TB, ctx context.Context, owner, project, token
 func (g GL) GetJobStatus(t testing.TB, ctx context.Context, owner, project, token string, jobID int) (string, error) {
 	git, err := gitlab.NewClient(token)
 	if err != nil {
-		return "", fmt.Errorf("Failed to create client: %v", err)
+		return "", fmt.Errorf("failed to create client: %v", err)
 	}
 
 	job, _, err := git.Jobs.GetJob(fmt.Sprintf("%s/%s", owner, project), jobID, gitlab.WithContext(ctx))
 	if err != nil {
-		return "", fmt.Errorf("Error retrying job: %v", err)
+		return "", fmt.Errorf("error retrying job: %v", err)
 	}
 
 	return job.Status, nil
@@ -232,7 +227,7 @@ func (g GL) AddProjectsToJobTokenScope(t testing.TB, owner, cicdProject, token s
 	ctx := context.Background()
 	git, err := gitlab.NewClient(token)
 	if err != nil {
-		return fmt.Errorf("Failed to create client: %v", err)
+		return fmt.Errorf("failed to create client: %v", err)
 	}
 	runnerProjectPath := fmt.Sprintf("%s/%s", owner, cicdProject)
 
@@ -243,13 +238,13 @@ func (g GL) AddProjectsToJobTokenScope(t testing.TB, owner, cicdProject, token s
 
 	runnerProjectID, err := getProjectIDByPath(git, ctx, runnerProjectPath)
 	if err != nil {
-		return fmt.Errorf("Could not get the project ID for the runner repository. Aborting. Error: %v", err)
+		return fmt.Errorf("could not get the project ID for the runner repository. Aborting. Error: %v", err)
 	}
 	for _, targetProjectPath := range projectsToAdd {
 
 		targetProjectID, err := getProjectIDByPath(git, ctx, targetProjectPath)
 		if err != nil {
-			return fmt.Errorf("Could not find project ID. Error: %v", err)
+			return fmt.Errorf("could not find project ID. Error: %v", err)
 		}
 
 		opts := &gitlab.JobTokenInboundAllowOptions{
@@ -266,7 +261,7 @@ func (g GL) AddProjectsToJobTokenScope(t testing.TB, owner, cicdProject, token s
 		}
 
 		if resp.StatusCode != http.StatusCreated {
-			return fmt.Errorf("DONE (Status: %s)\n", resp.Status)
+			return fmt.Errorf("done (Status: %s)", resp.Status)
 		}
 	}
 

helpers/foundation-deployer/main.go

@@ -188,7 +188,7 @@ func main() {
 			"TF_VAR_gh_token": conf.GitToken,
 		}
 	}
-	if conf.BuildType != stages.BuildTypeGitLab {
+	if conf.BuildType == stages.BuildTypeGitLab {
 		envVars = map[string]string{
 			"TF_VAR_gitlab_token": conf.GitToken,
 		}