diff --git a/cli/templatepush.go b/cli/templatepush.go index 7a21a0f8de..03e1ca1cee 100644 --- a/cli/templatepush.go +++ b/cli/templatepush.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "slices" + "strconv" "strings" "time" @@ -461,10 +462,14 @@ func createValidTemplateVersion(inv *serpent.Invocation, args createValidTemplat }) if err != nil { var jobErr *cliui.ProvisionerJobError - if errors.As(err, &jobErr) && !codersdk.JobIsMissingParameterErrorCode(jobErr.Code) { - return nil, err + if errors.As(err, &jobErr) { + if codersdk.JobIsMissingRequiredTemplateVariableErrorCode(jobErr.Code) { + return handleMissingTemplateVariables(inv, args, version.ID) + } + if !codersdk.JobIsMissingParameterErrorCode(jobErr.Code) { + return nil, err + } } - return nil, err } version, err = client.TemplateVersion(inv.Context(), version.ID) @@ -528,3 +533,153 @@ func prettyDirectoryPath(dir string) string { } return prettyDir } + +func handleMissingTemplateVariables(inv *serpent.Invocation, args createValidTemplateVersionArgs, failedVersionID uuid.UUID) (*codersdk.TemplateVersion, error) { + client := args.Client + + templateVariables, err := client.TemplateVersionVariables(inv.Context(), failedVersionID) + if err != nil { + return nil, xerrors.Errorf("fetch template variables: %w", err) + } + + existingValues := make(map[string]string) + for _, v := range args.UserVariableValues { + existingValues[v.Name] = v.Value + } + + var missingVariables []codersdk.TemplateVersionVariable + for _, variable := range templateVariables { + if !variable.Required { + continue + } + + if existingValue, exists := existingValues[variable.Name]; exists && existingValue != "" { + continue + } + + // Only prompt for variables that don't have a default value or have a redacted default + // Sensitive variables have a default value of "*redacted*" + // See: https://github.com/coder/coder/blob/a78790c632974e04babfef6de0e2ddf044787a7a/coderd/provisionerdserver/provisionerdserver.go#L3206 + if variable.DefaultValue == "" || (variable.Sensitive && variable.DefaultValue == "*redacted*") { + missingVariables = append(missingVariables, variable) + } + } + + if len(missingVariables) == 0 { + return nil, xerrors.New("no missing required variables found") + } + + _, _ = fmt.Fprintf(inv.Stderr, "Found %d missing required variables:\n", len(missingVariables)) + for _, v := range missingVariables { + _, _ = fmt.Fprintf(inv.Stderr, " - %s (%s): %s\n", v.Name, v.Type, v.Description) + } + + _, _ = fmt.Fprintln(inv.Stderr, "\nThe template requires values for the following variables:") + + var promptedValues []codersdk.VariableValue + for _, variable := range missingVariables { + value, err := promptForTemplateVariable(inv, variable) + if err != nil { + return nil, xerrors.Errorf("prompt for variable %q: %w", variable.Name, err) + } + promptedValues = append(promptedValues, codersdk.VariableValue{ + Name: variable.Name, + Value: value, + }) + } + + combinedValues := codersdk.CombineVariableValues(args.UserVariableValues, promptedValues) + + _, _ = fmt.Fprintln(inv.Stderr, "\nRetrying template build with provided variables...") + + retryArgs := args + retryArgs.UserVariableValues = combinedValues + + return createValidTemplateVersion(inv, retryArgs) +} + +func promptForTemplateVariable(inv *serpent.Invocation, variable codersdk.TemplateVersionVariable) (string, error) { + displayVariableInfo(inv, variable) + + switch variable.Type { + case "bool": + return promptForBoolVariable(inv, variable) + case "number": + return promptForNumberVariable(inv, variable) + default: + return promptForStringVariable(inv, variable) + } +} + +func displayVariableInfo(inv *serpent.Invocation, variable codersdk.TemplateVersionVariable) { + _, _ = fmt.Fprintf(inv.Stderr, "var.%s", cliui.Bold(variable.Name)) + if variable.Required { + _, _ = fmt.Fprint(inv.Stderr, pretty.Sprint(cliui.DefaultStyles.Error, " (required)")) + } + if variable.Sensitive { + _, _ = fmt.Fprint(inv.Stderr, pretty.Sprint(cliui.DefaultStyles.Warn, ", sensitive")) + } + _, _ = fmt.Fprintln(inv.Stderr, "") + + if variable.Description != "" { + _, _ = fmt.Fprintf(inv.Stderr, " Description: %s\n", variable.Description) + } + _, _ = fmt.Fprintf(inv.Stderr, " Type: %s\n", variable.Type) + _, _ = fmt.Fprintf(inv.Stderr, " Current value: %s\n", pretty.Sprint(cliui.DefaultStyles.Placeholder, "")) +} + +func promptForBoolVariable(inv *serpent.Invocation, variable codersdk.TemplateVersionVariable) (string, error) { + defaultValue := variable.DefaultValue + if defaultValue == "" { + defaultValue = "false" + } + + return cliui.Select(inv, cliui.SelectOptions{ + Options: []string{"true", "false"}, + Default: defaultValue, + Message: "Select value:", + }) +} + +func promptForNumberVariable(inv *serpent.Invocation, variable codersdk.TemplateVersionVariable) (string, error) { + prompt := "Enter value:" + if !variable.Required && variable.DefaultValue != "" { + prompt = fmt.Sprintf("Enter value (default: %q):", variable.DefaultValue) + } + + return cliui.Prompt(inv, cliui.PromptOptions{ + Text: prompt, + Default: variable.DefaultValue, + Validate: createVariableValidator(variable), + }) +} + +func promptForStringVariable(inv *serpent.Invocation, variable codersdk.TemplateVersionVariable) (string, error) { + prompt := "Enter value:" + if !variable.Sensitive { + if !variable.Required && variable.DefaultValue != "" { + prompt = fmt.Sprintf("Enter value (default: %q):", variable.DefaultValue) + } + } + + return cliui.Prompt(inv, cliui.PromptOptions{ + Text: prompt, + Default: variable.DefaultValue, + Secret: variable.Sensitive, + Validate: createVariableValidator(variable), + }) +} + +func createVariableValidator(variable codersdk.TemplateVersionVariable) func(string) error { + return func(s string) error { + if variable.Required && s == "" && variable.DefaultValue == "" { + return xerrors.New("value is required") + } + if variable.Type == "number" && s != "" { + if _, err := strconv.ParseFloat(s, 64); err != nil { + return xerrors.Errorf("must be a valid number, got: %q", s) + } + } + return nil + } +} diff --git a/cli/templatepush_test.go b/cli/templatepush_test.go index 7c8007c96a..28c5adc20f 100644 --- a/cli/templatepush_test.go +++ b/cli/templatepush_test.go @@ -852,54 +852,6 @@ func TestTemplatePush(t *testing.T) { require.Equal(t, "foobar", templateVariables[1].Value) }) - t.Run("VariableIsRequiredButNotProvided", func(t *testing.T) { - t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) - owner := coderdtest.CreateFirstUser(t, client) - templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) - - templateVersion := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, createEchoResponsesWithTemplateVariables(initialTemplateVariables)) - _ = coderdtest.AwaitTemplateVersionJobCompleted(t, client, templateVersion.ID) - template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, templateVersion.ID) - - // Test the cli command. - //nolint:gocritic - modifiedTemplateVariables := append(initialTemplateVariables, - &proto.TemplateVariable{ - Name: "second_variable", - Description: "This is the second variable.", - Type: "string", - Required: true, - }, - ) - source := clitest.CreateTemplateVersionSource(t, createEchoResponsesWithTemplateVariables(modifiedTemplateVariables)) - inv, root := clitest.New(t, "templates", "push", template.Name, "--directory", source, "--test.provisioner", string(database.ProvisionerTypeEcho), "--name", "example") - clitest.SetupConfig(t, templateAdmin, root) - pty := ptytest.New(t) - inv.Stdin = pty.Input() - inv.Stdout = pty.Output() - - execDone := make(chan error) - go func() { - execDone <- inv.Run() - }() - - matches := []struct { - match string - write string - }{ - {match: "Upload", write: "yes"}, - } - for _, m := range matches { - pty.ExpectMatch(m.match) - pty.WriteLine(m.write) - } - - wantErr := <-execDone - require.Error(t, wantErr) - require.Contains(t, wantErr.Error(), "required template variables need values") - }) - t.Run("VariableIsOptionalButNotProvided", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) @@ -1115,6 +1067,240 @@ func TestTemplatePush(t *testing.T) { require.Len(t, templateVersions, 2) require.Equal(t, "example", templateVersions[1].Name) }) + + t.Run("PromptForDifferentRequiredTypes", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + owner := coderdtest.CreateFirstUser(t, client) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) + + templateVariables := []*proto.TemplateVariable{ + { + Name: "string_var", + Description: "A string variable", + Type: "string", + Required: true, + }, + { + Name: "number_var", + Description: "A number variable", + Type: "number", + Required: true, + }, + { + Name: "bool_var", + Description: "A boolean variable", + Type: "bool", + Required: true, + }, + { + Name: "sensitive_var", + Description: "A sensitive variable", + Type: "string", + Required: true, + Sensitive: true, + }, + } + + source := clitest.CreateTemplateVersionSource(t, createEchoResponsesWithTemplateVariables(templateVariables)) + inv, root := clitest.New(t, "templates", "push", "test-template", "--directory", source, "--test.provisioner", string(database.ProvisionerTypeEcho)) + clitest.SetupConfig(t, templateAdmin, root) + pty := ptytest.New(t).Attach(inv) + + execDone := make(chan error) + go func() { + execDone <- inv.Run() + }() + + // Select "Yes" for the "Upload " prompt + pty.ExpectMatch("Upload") + pty.WriteLine("yes") + + pty.ExpectMatch("var.string_var") + pty.ExpectMatch("Enter value:") + pty.WriteLine("test-string") + + pty.ExpectMatch("var.number_var") + pty.ExpectMatch("Enter value:") + pty.WriteLine("42") + + // Boolean variable automatically selects the first option ("true") + pty.ExpectMatch("var.bool_var") + + pty.ExpectMatch("var.sensitive_var") + pty.ExpectMatch("Enter value:") + pty.WriteLine("secret-value") + + require.NoError(t, <-execDone) + }) + + t.Run("ValidateNumberInput", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + owner := coderdtest.CreateFirstUser(t, client) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) + + templateVariables := []*proto.TemplateVariable{ + { + Name: "number_var", + Description: "A number that requires validation", + Type: "number", + Required: true, + }, + } + + source := clitest.CreateTemplateVersionSource(t, createEchoResponsesWithTemplateVariables(templateVariables)) + inv, root := clitest.New(t, "templates", "push", "test-template", "--directory", source, "--test.provisioner", string(database.ProvisionerTypeEcho)) + clitest.SetupConfig(t, templateAdmin, root) + pty := ptytest.New(t).Attach(inv) + + execDone := make(chan error) + go func() { + execDone <- inv.Run() + }() + + // Select "Yes" for the "Upload " prompt + pty.ExpectMatch("Upload") + pty.WriteLine("yes") + + pty.ExpectMatch("var.number_var") + + pty.WriteLine("not-a-number") + pty.ExpectMatch("must be a valid number") + + pty.WriteLine("123.45") + + require.NoError(t, <-execDone) + }) + + t.Run("DontPromptForDefaultValues", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + owner := coderdtest.CreateFirstUser(t, client) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) + + templateVariables := []*proto.TemplateVariable{ + { + Name: "with_default", + Type: "string", + Required: true, + DefaultValue: "default-value", + }, + { + Name: "without_default", + Type: "string", + Required: true, + }, + } + + source := clitest.CreateTemplateVersionSource(t, createEchoResponsesWithTemplateVariables(templateVariables)) + inv, root := clitest.New(t, "templates", "push", "test-template", "--directory", source, "--test.provisioner", string(database.ProvisionerTypeEcho)) + clitest.SetupConfig(t, templateAdmin, root) + pty := ptytest.New(t).Attach(inv) + + execDone := make(chan error) + go func() { + execDone <- inv.Run() + }() + + // Select "Yes" for the "Upload " prompt + pty.ExpectMatch("Upload") + pty.WriteLine("yes") + + pty.ExpectMatch("var.without_default") + pty.WriteLine("test-value") + + require.NoError(t, <-execDone) + }) + + t.Run("VariableSourcesPriority", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + owner := coderdtest.CreateFirstUser(t, client) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) + + templateVariables := []*proto.TemplateVariable{ + { + Name: "cli_flag_var", + Description: "Variable provided via CLI flag", + Type: "string", + Required: true, + }, + { + Name: "file_var", + Description: "Variable provided via file", + Type: "string", + Required: true, + }, + { + Name: "prompt_var", + Description: "Variable provided via prompt", + Type: "string", + Required: true, + }, + { + Name: "cli_overrides_file_var", + Description: "Variable in both CLI and file", + Type: "string", + Required: true, + }, + } + + source := clitest.CreateTemplateVersionSource(t, createEchoResponsesWithTemplateVariables(templateVariables)) + + // Create a temporary variables file. + tempDir := t.TempDir() + removeTmpDirUntilSuccessAfterTest(t, tempDir) + variablesFile, err := os.CreateTemp(tempDir, "variables*.yaml") + require.NoError(t, err) + _, err = variablesFile.WriteString(`file_var: from-file +cli_overrides_file_var: from-file`) + require.NoError(t, err) + require.NoError(t, variablesFile.Close()) + + inv, root := clitest.New(t, "templates", "push", "test-template", + "--directory", source, + "--test.provisioner", string(database.ProvisionerTypeEcho), + "--variables-file", variablesFile.Name(), + "--variable", "cli_flag_var=from-cli-flag", + "--variable", "cli_overrides_file_var=from-cli-override", + ) + clitest.SetupConfig(t, templateAdmin, root) + pty := ptytest.New(t).Attach(inv) + + execDone := make(chan error) + go func() { + execDone <- inv.Run() + }() + + // Select "Yes" for the "Upload " prompt + pty.ExpectMatch("Upload") + pty.WriteLine("yes") + + // Only check for prompt_var, other variables should not prompt + pty.ExpectMatch("var.prompt_var") + pty.ExpectMatch("Enter value:") + pty.WriteLine("from-prompt") + + require.NoError(t, <-execDone) + + template, err := client.TemplateByName(context.Background(), owner.OrganizationID, "test-template") + require.NoError(t, err) + + templateVersionVars, err := client.TemplateVersionVariables(context.Background(), template.ActiveVersionID) + require.NoError(t, err) + require.Len(t, templateVersionVars, 4) + + varMap := make(map[string]string) + for _, tv := range templateVersionVars { + varMap[tv.Name] = tv.Value + } + + require.Equal(t, "from-cli-flag", varMap["cli_flag_var"]) + require.Equal(t, "from-file", varMap["file_var"]) + require.Equal(t, "from-prompt", varMap["prompt_var"]) + require.Equal(t, "from-cli-override", varMap["cli_overrides_file_var"]) + }) }) } diff --git a/codersdk/provisionerdaemons.go b/codersdk/provisionerdaemons.go index 4bff7d7827..b3cefa0911 100644 --- a/codersdk/provisionerdaemons.go +++ b/codersdk/provisionerdaemons.go @@ -175,6 +175,12 @@ func JobIsMissingParameterErrorCode(code JobErrorCode) bool { return string(code) == runner.MissingParameterErrorCode } +// JobIsMissingRequiredTemplateVariableErrorCode returns whether the error is a missing a required template +// variable error. This can indicate to consumers that they need to provide required template variables. +func JobIsMissingRequiredTemplateVariableErrorCode(code JobErrorCode) bool { + return string(code) == runner.RequiredTemplateVariablesErrorCode +} + // ProvisionerJob describes the job executed by the provisioning daemon. type ProvisionerJob struct { ID uuid.UUID `json:"id" format:"uuid" table:"id"` diff --git a/codersdk/templatevariables.go b/codersdk/templatevariables.go index 3e02f69106..19c614e796 100644 --- a/codersdk/templatevariables.go +++ b/codersdk/templatevariables.go @@ -68,7 +68,7 @@ func ParseUserVariableValues(varsFiles []string, variablesFile string, commandLi return nil, err } - return combineVariableValues(fromVars, fromFile, fromCommandLine), nil + return CombineVariableValues(fromVars, fromFile, fromCommandLine), nil } func parseVariableValuesFromVarsFiles(varsFiles []string) ([]VariableValue, error) { @@ -252,7 +252,7 @@ func parseVariableValuesFromCommandLine(variables []string) ([]VariableValue, er return values, nil } -func combineVariableValues(valuesSets ...[]VariableValue) []VariableValue { +func CombineVariableValues(valuesSets ...[]VariableValue) []VariableValue { combinedValues := make(map[string]string) for _, values := range valuesSets {