From 23c4aab9d2b1b4d206361d0cad027d0c8a0ebd54 Mon Sep 17 00:00:00 2001 From: Luke Kysow <1034429+lkysow@users.noreply.github.com> Date: Mon, 20 Jan 2020 09:17:01 -0600 Subject: [PATCH] Refactor -tf-download-url flag changes. * Fix bad ordering of args calling NewClient() * Make error messages clearer --- cmd/server.go | 2 +- server/events/terraform/terraform_client.go | 7 ++++--- .../events/terraform/terraform_client_test.go | 20 +++++++++---------- server/events_controller_e2e_test.go | 2 +- server/server.go | 2 +- 5 files changed, 17 insertions(+), 16 deletions(-) diff --git a/cmd/server.go b/cmd/server.go index 9b6caf5313..500c988b33 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -205,7 +205,7 @@ var stringFlags = map[string]stringFlag{ description: fmt.Sprintf("File containing x509 private key matching --%s.", SSLCertFileFlag), }, TFDownloadURLFlag: { - description: "URL to download Terraform from.", + description: "Base URL to download Terraform versions from.", defaultValue: DefaultTFDownloadURL, }, TFEHostnameFlag: { diff --git a/server/events/terraform/terraform_client.go b/server/events/terraform/terraform_client.go index 5f88892014..68e39d6abb 100644 --- a/server/events/terraform/terraform_client.go +++ b/server/events/terraform/terraform_client.go @@ -148,7 +148,7 @@ func NewClient( _, err := ensureVersion(log, tfDownloader, versions, defaultVersion, binDir, tfDownloadURL) versionsLock.Unlock() if err != nil { - log.Err("could not download terraform %s", defaultVersion.String()) + log.Err("could not download terraform %s: %s", defaultVersion.String(), err) } }() } @@ -416,8 +416,9 @@ func ensureVersion(log *logging.SimpleLogger, dl Downloader, versions map[string urlPrefix := fmt.Sprintf("%s/terraform/%s/terraform_%s", downloadURL, v.String(), v.String()) binURL := fmt.Sprintf("%s_%s_%s.zip", urlPrefix, runtime.GOOS, runtime.GOARCH) checksumURL := fmt.Sprintf("%s_SHA256SUMS", urlPrefix) - if err := dl.GetFile(dest, fmt.Sprintf("%s?checksum=file:%s", binURL, checksumURL)); err != nil { - return "", errors.Wrapf(err, "downloading terraform version %s", v.String()) + fullSrcURL := fmt.Sprintf("%s?checksum=file:%s", binURL, checksumURL) + if err := dl.GetFile(dest, fullSrcURL); err != nil { + return "", errors.Wrapf(err, "downloading terraform version %s at %q", v.String(), fullSrcURL) } log.Info("downloaded terraform %s to %s", v.String(), dest) diff --git a/server/events/terraform/terraform_client_test.go b/server/events/terraform/terraform_client_test.go index 49ca9dd8ba..00eb73032b 100644 --- a/server/events/terraform/terraform_client_test.go +++ b/server/events/terraform/terraform_client_test.go @@ -68,7 +68,7 @@ is 0.11.13. You can update by downloading from www.terraform.io/downloads.html Ok(t, err) defer tempSetEnv(t, "PATH", fmt.Sprintf("%s:%s", tmp, os.Getenv("PATH")))() - c, err := terraform.NewClient(nil, tmp, "", "", "", cmd.DefaultTFVersionFlag, "https://releases.hashicorp.com", nil) + c, err := terraform.NewClient(nil, tmp, "", "", "", cmd.DefaultTFVersionFlag, cmd.DefaultTFDownloadURL, nil) Ok(t, err) Ok(t, err) @@ -96,7 +96,7 @@ is 0.11.13. You can update by downloading from www.terraform.io/downloads.html Ok(t, err) defer tempSetEnv(t, "PATH", fmt.Sprintf("%s:%s", tmp, os.Getenv("PATH")))() - c, err := terraform.NewClient(nil, tmp, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.TFDownloadURLFlag, nil) + c, err := terraform.NewClient(nil, tmp, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.DefaultTFDownloadURL, nil) Ok(t, err) Ok(t, err) @@ -116,7 +116,7 @@ func TestNewClient_NoTF(t *testing.T) { // Set PATH to only include our empty directory. defer tempSetEnv(t, "PATH", tmp)() - _, err := terraform.NewClient(nil, tmp, "", "", "", cmd.DefaultTFVersionFlag, cmd.TFDownloadURLFlag, nil) + _, err := terraform.NewClient(nil, tmp, "", "", "", cmd.DefaultTFVersionFlag, cmd.DefaultTFDownloadURL, nil) ErrEquals(t, "terraform not found in $PATH. Set --default-tf-version or download terraform from https://www.terraform.io/downloads.html", err) } @@ -133,7 +133,7 @@ func TestNewClient_DefaultTFFlagInPath(t *testing.T) { Ok(t, err) defer tempSetEnv(t, "PATH", fmt.Sprintf("%s:%s", tmp, os.Getenv("PATH")))() - c, err := terraform.NewClient(nil, tmp, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.TFDownloadURLFlag, nil) + c, err := terraform.NewClient(nil, tmp, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.DefaultTFDownloadURL, nil) Ok(t, err) Ok(t, err) @@ -157,7 +157,7 @@ func TestNewClient_DefaultTFFlagInBinDir(t *testing.T) { Ok(t, err) defer tempSetEnv(t, "PATH", fmt.Sprintf("%s:%s", tmp, os.Getenv("PATH")))() - c, err := terraform.NewClient(logging.NewNoopLogger(), tmp, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.TFDownloadURLFlag, nil) + c, err := terraform.NewClient(logging.NewNoopLogger(), tmp, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.DefaultTFDownloadURL, nil) Ok(t, err) Ok(t, err) @@ -207,7 +207,7 @@ func TestNewClient_DefaultTFFlagDownload(t *testing.T) { func TestNewClient_BadVersion(t *testing.T) { tmp, cleanup := TempDir(t) defer cleanup() - _, err := terraform.NewClient(nil, tmp, "", "", "malformed", cmd.DefaultTFVersionFlag, cmd.TFDownloadURLFlag, nil) + _, err := terraform.NewClient(nil, tmp, "", "", "malformed", cmd.DefaultTFVersionFlag, cmd.DefaultTFDownloadURL, nil) ErrEquals(t, "Malformed version: malformed", err) } @@ -219,7 +219,7 @@ func TestRunCommandWithVersion_DLsTF(t *testing.T) { mockDownloader := mocks.NewMockDownloader() // Set up our mock downloader to write a fake tf binary when it's called. - baseURL := fmt.Sprintf("%s/terraform/99.99.99", cmd.TFDownloadURLFlag) + baseURL := fmt.Sprintf("%s/terraform/99.99.99", cmd.DefaultTFDownloadURL) expURL := fmt.Sprintf("%s/terraform_99.99.99_%s_%s.zip?checksum=file:%s/terraform_99.99.99_SHA256SUMS", baseURL, runtime.GOOS, @@ -230,7 +230,7 @@ func TestRunCommandWithVersion_DLsTF(t *testing.T) { return []pegomock.ReturnValue{err} }) - c, err := terraform.NewClient(nil, tmp, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.TFDownloadURLFlag, mockDownloader) + c, err := terraform.NewClient(nil, tmp, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.DefaultTFDownloadURL, mockDownloader) Ok(t, err) Equals(t, "0.11.10", c.DefaultVersion().String()) @@ -249,7 +249,7 @@ func TestEnsureVersion_downloaded(t *testing.T) { mockDownloader := mocks.NewMockDownloader() - c, err := terraform.NewClient(nil, tmp, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.TFDownloadURLFlag, mockDownloader) + c, err := terraform.NewClient(nil, tmp, "", "", "0.11.10", cmd.DefaultTFVersionFlag, cmd.DefaultTFDownloadURL, mockDownloader) Ok(t, err) Equals(t, "0.11.10", c.DefaultVersion().String()) @@ -261,7 +261,7 @@ func TestEnsureVersion_downloaded(t *testing.T) { Ok(t, err) - baseURL := fmt.Sprintf("%s/terraform/99.99.99", cmd.TFDownloadURLFlag) + baseURL := fmt.Sprintf("%s/terraform/99.99.99", cmd.DefaultTFDownloadURL) expURL := fmt.Sprintf("%s/terraform_99.99.99_%s_%s.zip?checksum=file:%s/terraform_99.99.99_SHA256SUMS", baseURL, runtime.GOOS, diff --git a/server/events_controller_e2e_test.go b/server/events_controller_e2e_test.go index 275d2a9fd9..a0b4d12589 100644 --- a/server/events_controller_e2e_test.go +++ b/server/events_controller_e2e_test.go @@ -400,7 +400,7 @@ func setupE2E(t *testing.T, repoDir string) (server.EventsController, *vcsmocks. GithubUser: "github-user", GitlabUser: "gitlab-user", } - terraformClient, err := terraform.NewClient(logger, dataDir, "", "", "", "tfdownloadurl", "default-tf-version", &NoopTFDownloader{}) + terraformClient, err := terraform.NewClient(logger, dataDir, "", "", "", "default-tf-version", "https://releases.hashicorp.com", &NoopTFDownloader{}) Ok(t, err) boltdb, err := db.New(dataDir) Ok(t, err) diff --git a/server/server.go b/server/server.go index bec83a7784..f8ecab83fe 100644 --- a/server/server.go +++ b/server/server.go @@ -211,9 +211,9 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { userConfig.DataDir, userConfig.TFEToken, userConfig.TFEHostname, - userConfig.TFDownloadURL, userConfig.DefaultTFVersion, config.DefaultTFVersionFlag, + userConfig.TFDownloadURL, &terraform.DefaultDownloader{}) // The flag.Lookup call is to detect if we're running in a unit test. If we // are, then we don't error out because we don't have/want terraform