From adf14f1917be1982eea858f4591411f2e7c3d45c Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Tue, 20 Jun 2023 10:02:44 -0500 Subject: [PATCH] chore(cli): warn on template push or create when no lockfile present (#8059) --- cli/clitest/clitest.go | 6 +- cli/templatecreate.go | 7 +- cli/templatecreate_test.go | 82 +++++++++++++++++++ cli/templatepush.go | 41 ++++++++-- cli/templatepush_test.go | 81 ++++++++++++++++++ .../coder_templates_create_--help.golden | 4 + .../coder_templates_push_--help.golden | 4 + docs/cli/templates_create.md | 9 ++ docs/cli/templates_push.md | 9 ++ provisionersdk/archive.go | 4 + pty/ptytest/ptytest.go | 33 ++++++++ 11 files changed, 272 insertions(+), 8 deletions(-) diff --git a/cli/clitest/clitest.go b/cli/clitest/clitest.go index 4862db81d6..00ec131004 100644 --- a/cli/clitest/clitest.go +++ b/cli/clitest/clitest.go @@ -6,7 +6,6 @@ import ( "context" "errors" "io" - "io/ioutil" "os" "path/filepath" "strings" @@ -86,7 +85,10 @@ func SetupConfig(t *testing.T, client *codersdk.Client, root config.Root) { // new temporary testing directory. func CreateTemplateVersionSource(t *testing.T, responses *echo.Responses) string { directory := t.TempDir() - f, err := ioutil.TempFile(directory, "*.tf") + f, err := os.CreateTemp(directory, "*.tf") + require.NoError(t, err) + _ = f.Close() + f, err = os.Create(filepath.Join(directory, ".terraform.lock.hcl")) require.NoError(t, err) _ = f.Close() data, err := echo.Tar(responses) diff --git a/cli/templatecreate.go b/cli/templatecreate.go index eb23b10fdb..555707b28b 100644 --- a/cli/templatecreate.go +++ b/cli/templatecreate.go @@ -87,6 +87,11 @@ func (r *RootCmd) templateCreate() *clibase.Cmd { return xerrors.Errorf("A template already exists named %q!", templateName) } + err = uploadFlags.checkForLockfile(inv) + if err != nil { + return xerrors.Errorf("check for lockfile: %w", err) + } + // Confirm upload of the directory. resp, err := uploadFlags.upload(inv, client) if err != nil { @@ -185,7 +190,6 @@ func (r *RootCmd) templateCreate() *clibase.Cmd { Default: "0h", Value: clibase.DurationOf(&inactivityTTL), }, - uploadFlags.option(), { Flag: "test.provisioner", Description: "Customize the provisioner backend.", @@ -195,6 +199,7 @@ func (r *RootCmd) templateCreate() *clibase.Cmd { }, cliui.SkipPromptOption(), } + cmd.Options = append(cmd.Options, uploadFlags.options()...) return cmd } diff --git a/cli/templatecreate_test.go b/cli/templatecreate_test.go index 152c2d3920..06e180f7dc 100644 --- a/cli/templatecreate_test.go +++ b/cli/templatecreate_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -80,6 +81,87 @@ func TestTemplateCreate(t *testing.T) { } } }) + t.Run("CreateNoLockfile", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + coderdtest.CreateFirstUser(t, client) + source := clitest.CreateTemplateVersionSource(t, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionApply: provisionCompleteWithAgent, + }) + require.NoError(t, os.Remove(filepath.Join(source, ".terraform.lock.hcl"))) + args := []string{ + "templates", + "create", + "my-template", + "--directory", source, + "--test.provisioner", string(database.ProvisionerTypeEcho), + "--default-ttl", "24h", + } + inv, root := clitest.New(t, args...) + clitest.SetupConfig(t, client, root) + pty := ptytest.New(t).Attach(inv) + + execDone := make(chan error) + go func() { + execDone <- inv.Run() + }() + + matches := []struct { + match string + write string + }{ + {match: "No .terraform.lock.hcl file found"}, + {match: "Upload", write: "no"}, + } + for _, m := range matches { + pty.ExpectMatch(m.match) + if len(m.write) > 0 { + pty.WriteLine(m.write) + } + } + + // cmd should error once we say no. + require.Error(t, <-execDone) + }) + t.Run("CreateNoLockfileIgnored", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + coderdtest.CreateFirstUser(t, client) + source := clitest.CreateTemplateVersionSource(t, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionApply: provisionCompleteWithAgent, + }) + require.NoError(t, os.Remove(filepath.Join(source, ".terraform.lock.hcl"))) + args := []string{ + "templates", + "create", + "my-template", + "--directory", source, + "--test.provisioner", string(database.ProvisionerTypeEcho), + "--default-ttl", "24h", + "--ignore-lockfile", + } + inv, root := clitest.New(t, args...) + clitest.SetupConfig(t, client, root) + pty := ptytest.New(t).Attach(inv) + + execDone := make(chan error) + go func() { + execDone <- inv.Run() + }() + + { + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) + defer cancel() + + pty.ExpectNoMatchBefore(ctx, "No .terraform.lock.hcl file found", "Upload") + pty.WriteLine("no") + } + + // cmd should error once we say no. + require.Error(t, <-execDone) + }) t.Run("CreateStdin", func(t *testing.T) { t.Parallel() diff --git a/cli/templatepush.go b/cli/templatepush.go index 29201b8510..02884aed36 100644 --- a/cli/templatepush.go +++ b/cli/templatepush.go @@ -19,17 +19,23 @@ import ( // templateUploadFlags is shared by `templates create` and `templates push`. type templateUploadFlags struct { - directory string + directory string + ignoreLockfile bool } -func (pf *templateUploadFlags) option() clibase.Option { - return clibase.Option{ +func (pf *templateUploadFlags) options() []clibase.Option { + return []clibase.Option{{ Flag: "directory", FlagShorthand: "d", Description: "Specify the directory to create from, use '-' to read tar from stdin.", Default: ".", Value: clibase.StringOf(&pf.directory), - } + }, { + Flag: "ignore-lockfile", + Description: "Ignore warnings about not having a .terraform.lock.hcl file present in the template.", + Default: "false", + Value: clibase.BoolOf(&pf.ignoreLockfile), + }} } func (pf *templateUploadFlags) setWorkdir(wd string) { @@ -84,6 +90,26 @@ func (pf *templateUploadFlags) upload(inv *clibase.Invocation, client *codersdk. return &resp, nil } +func (pf *templateUploadFlags) checkForLockfile(inv *clibase.Invocation) error { + if pf.stdin() || pf.ignoreLockfile { + // Just assume there's a lockfile if reading from stdin. + return nil + } + + hasLockfile, err := provisionersdk.DirHasLockfile(pf.directory) + if err != nil { + return xerrors.Errorf("dir has lockfile: %w", err) + } + + if !hasLockfile { + cliui.Warn(inv.Stdout, "No .terraform.lock.hcl file found", + "When provisioning, Coder will be unable to cache providers without a lockfile and must download them from the internet each time.", + "Create one by running "+cliui.DefaultStyles.Code.Render("terraform init")+" in your template directory.", + ) + } + return nil +} + func (pf *templateUploadFlags) templateName(args []string) (string, error) { if pf.stdin() { // Can't infer name from directory if none provided. @@ -143,6 +169,11 @@ func (r *RootCmd) templatePush() *clibase.Cmd { return err } + err = uploadFlags.checkForLockfile(inv) + if err != nil { + return xerrors.Errorf("check for lockfile: %w", err) + } + resp, err := uploadFlags.upload(inv, client) if err != nil { return err @@ -236,7 +267,7 @@ func (r *RootCmd) templatePush() *clibase.Cmd { Value: clibase.BoolOf(&activate), }, cliui.SkipPromptOption(), - uploadFlags.option(), } + cmd.Options = append(cmd.Options, uploadFlags.options()...) return cmd } diff --git a/cli/templatepush_test.go b/cli/templatepush_test.go index 82b0fd5198..22f5b6a38f 100644 --- a/cli/templatepush_test.go +++ b/cli/templatepush_test.go @@ -18,6 +18,7 @@ import ( "github.com/coder/coder/provisioner/echo" "github.com/coder/coder/provisionersdk/proto" "github.com/coder/coder/pty/ptytest" + "github.com/coder/coder/testutil" ) func TestTemplatePush(t *testing.T) { @@ -69,6 +70,86 @@ func TestTemplatePush(t *testing.T) { require.Equal(t, "example", templateVersions[1].Name) }) + t.Run("NoLockfile", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + + // Test the cli command. + source := clitest.CreateTemplateVersionSource(t, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionApply: echo.ProvisionComplete, + }) + require.NoError(t, os.Remove(filepath.Join(source, ".terraform.lock.hcl"))) + + inv, root := clitest.New(t, "templates", "push", template.Name, "--directory", source, "--test.provisioner", string(database.ProvisionerTypeEcho), "--name", "example") + clitest.SetupConfig(t, client, root) + pty := ptytest.New(t).Attach(inv) + + execDone := make(chan error) + go func() { + execDone <- inv.Run() + }() + + matches := []struct { + match string + write string + }{ + {match: "No .terraform.lock.hcl file found"}, + {match: "Upload", write: "no"}, + } + for _, m := range matches { + pty.ExpectMatch(m.match) + if m.write != "" { + pty.WriteLine(m.write) + } + } + + // cmd should error once we say no. + require.Error(t, <-execDone) + }) + + t.Run("NoLockfileIgnored", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + + // Test the cli command. + source := clitest.CreateTemplateVersionSource(t, &echo.Responses{ + Parse: echo.ParseComplete, + ProvisionApply: echo.ProvisionComplete, + }) + require.NoError(t, os.Remove(filepath.Join(source, ".terraform.lock.hcl"))) + + inv, root := clitest.New(t, "templates", "push", template.Name, "--directory", source, "--test.provisioner", string(database.ProvisionerTypeEcho), "--name", "example", "--ignore-lockfile") + clitest.SetupConfig(t, client, root) + pty := ptytest.New(t).Attach(inv) + + execDone := make(chan error) + go func() { + execDone <- inv.Run() + }() + + { + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium) + defer cancel() + + pty.ExpectNoMatchBefore(ctx, "No .terraform.lock.hcl file found", "Upload") + pty.WriteLine("no") + } + + // cmd should error once we say no. + require.Error(t, <-execDone) + }) + t.Run("PushInactiveTemplateVersion", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) diff --git a/cli/testdata/coder_templates_create_--help.golden b/cli/testdata/coder_templates_create_--help.golden index cf6e5c9e3a..d23b0f7571 100644 --- a/cli/testdata/coder_templates_create_--help.golden +++ b/cli/testdata/coder_templates_create_--help.golden @@ -13,6 +13,10 @@ Create a template from the current directory or as specified by flag Specify a failure TTL for workspaces created from this template. This licensed feature's default is 0h (off). + --ignore-lockfile bool (default: false) + Ignore warnings about not having a .terraform.lock.hcl file present in + the template. + --inactivity-ttl duration (default: 0h) Specify an inactivity TTL for workspaces created from this template. This licensed feature's default is 0h (off). diff --git a/cli/testdata/coder_templates_push_--help.golden b/cli/testdata/coder_templates_push_--help.golden index c544ecc341..ebe0a73e70 100644 --- a/cli/testdata/coder_templates_push_--help.golden +++ b/cli/testdata/coder_templates_push_--help.golden @@ -13,6 +13,10 @@ Push a new template version from the current directory or as specified by flag -d, --directory string (default: .) Specify the directory to create from, use '-' to read tar from stdin. + --ignore-lockfile bool (default: false) + Ignore warnings about not having a .terraform.lock.hcl file present in + the template. + --name string Specify a name for the new template version. It will be automatically generated if not provided. diff --git a/docs/cli/templates_create.md b/docs/cli/templates_create.md index 123bd0bed9..197ba3fdcf 100644 --- a/docs/cli/templates_create.md +++ b/docs/cli/templates_create.md @@ -39,6 +39,15 @@ Specify the directory to create from, use '-' to read tar from stdin. Specify a failure TTL for workspaces created from this template. This licensed feature's default is 0h (off). +### --ignore-lockfile + +| | | +| ------- | ------------------ | +| Type | bool | +| Default | false | + +Ignore warnings about not having a .terraform.lock.hcl file present in the template. + ### --inactivity-ttl | | | diff --git a/docs/cli/templates_push.md b/docs/cli/templates_push.md index b3762a641e..692887b692 100644 --- a/docs/cli/templates_push.md +++ b/docs/cli/templates_push.md @@ -38,6 +38,15 @@ Always prompt all parameters. Does not pull parameter values from active templat Specify the directory to create from, use '-' to read tar from stdin. +### --ignore-lockfile + +| | | +| ------- | ------------------ | +| Type | bool | +| Default | false | + +Ignore warnings about not having a .terraform.lock.hcl file present in the template. + ### --name | | | diff --git a/provisionersdk/archive.go b/provisionersdk/archive.go index cc12c2109f..f54a0e7602 100644 --- a/provisionersdk/archive.go +++ b/provisionersdk/archive.go @@ -34,6 +34,10 @@ func dirHasExt(dir string, exts ...string) (bool, error) { return false, nil } +func DirHasLockfile(dir string) (bool, error) { + return dirHasExt(dir, ".terraform.lock.hcl") +} + // Tar archives a Terraform directory. func Tar(w io.Writer, directory string, limit int64) error { // The total bytes written must be under the limit, so use -1 diff --git a/pty/ptytest/ptytest.go b/pty/ptytest/ptytest.go index 5036668d82..b4091ec20e 100644 --- a/pty/ptytest/ptytest.go +++ b/pty/ptytest/ptytest.go @@ -181,6 +181,39 @@ func (e *outExpecter) ExpectMatchContext(ctx context.Context, str string) string return buffer.String() } +// ExpectNoMatchBefore validates that `match` does not occur before `before`. +func (e *outExpecter) ExpectNoMatchBefore(ctx context.Context, match, before string) string { + e.t.Helper() + + var buffer bytes.Buffer + err := e.doMatchWithDeadline(ctx, "ExpectNoMatchBefore", func() error { + for { + r, _, err := e.runeReader.ReadRune() + if err != nil { + return err + } + _, err = buffer.WriteRune(r) + if err != nil { + return err + } + + if strings.Contains(buffer.String(), match) { + return xerrors.Errorf("found %q before %q", match, before) + } + + if strings.Contains(buffer.String(), before) { + return nil + } + } + }) + if err != nil { + e.fatalf("read error", "%v (wanted no %q before %q; got %q)", err, match, before, buffer.String()) + return "" + } + e.logf("matched %q = %q", before, stripansi.Strip(buffer.String())) + return buffer.String() +} + func (e *outExpecter) Peek(ctx context.Context, n int) []byte { e.t.Helper()