Skip to content

feat: Updating workspace prompts new parameters #2598

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jun 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
202 changes: 121 additions & 81 deletions cli/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -120,87 +120,11 @@ func create() *cobra.Command {
schedSpec = ptr.Ref(sched.String())
}

templateVersion, err := client.TemplateVersion(cmd.Context(), template.ActiveVersionID)
if err != nil {
return err
}
parameterSchemas, err := client.TemplateVersionSchema(cmd.Context(), templateVersion.ID)
if err != nil {
return err
}

// parameterMapFromFile can be nil if parameter file is not specified
var parameterMapFromFile map[string]string
if parameterFile != "" {
_, _ = fmt.Fprintln(cmd.OutOrStdout(), cliui.Styles.Paragraph.Render("Attempting to read the variables from the parameter file.")+"\r\n")
parameterMapFromFile, err = createParameterMapFromFile(parameterFile)
if err != nil {
return err
}
}

disclaimerPrinted := false
parameters := make([]codersdk.CreateParameterRequest, 0)
for _, parameterSchema := range parameterSchemas {
if !parameterSchema.AllowOverrideSource {
continue
}
if !disclaimerPrinted {
_, _ = fmt.Fprintln(cmd.OutOrStdout(), cliui.Styles.Paragraph.Render("This template has customizable parameters. Values can be changed after create, but may have unintended side effects (like data loss).")+"\r\n")
disclaimerPrinted = true
}
parameterValue, err := getParameterValueFromMapOrInput(cmd, parameterMapFromFile, parameterSchema)
if err != nil {
return err
}
parameters = append(parameters, codersdk.CreateParameterRequest{
Name: parameterSchema.Name,
SourceValue: parameterValue,
SourceScheme: codersdk.ParameterSourceSchemeData,
DestinationScheme: parameterSchema.DefaultDestinationScheme,
})
}
_, _ = fmt.Fprintln(cmd.OutOrStdout())

// Run a dry-run with the given parameters to check correctness
after := time.Now()
dryRun, err := client.CreateTemplateVersionDryRun(cmd.Context(), templateVersion.ID, codersdk.CreateTemplateVersionDryRunRequest{
WorkspaceName: workspaceName,
ParameterValues: parameters,
})
if err != nil {
return xerrors.Errorf("begin workspace dry-run: %w", err)
}
_, _ = fmt.Fprintln(cmd.OutOrStdout(), "Planning workspace...")
err = cliui.ProvisionerJob(cmd.Context(), cmd.OutOrStdout(), cliui.ProvisionerJobOptions{
Fetch: func() (codersdk.ProvisionerJob, error) {
return client.TemplateVersionDryRun(cmd.Context(), templateVersion.ID, dryRun.ID)
},
Cancel: func() error {
return client.CancelTemplateVersionDryRun(cmd.Context(), templateVersion.ID, dryRun.ID)
},
Logs: func() (<-chan codersdk.ProvisionerJobLog, error) {
return client.TemplateVersionDryRunLogsAfter(cmd.Context(), templateVersion.ID, dryRun.ID, after)
},
// Don't show log output for the dry-run unless there's an error.
Silent: true,
})
if err != nil {
// TODO (Dean): reprompt for parameter values if we deem it to
// be a validation error
return xerrors.Errorf("dry-run workspace: %w", err)
}

resources, err := client.TemplateVersionDryRunResources(cmd.Context(), templateVersion.ID, dryRun.ID)
if err != nil {
return xerrors.Errorf("get workspace dry-run resources: %w", err)
}

err = cliui.WorkspaceResources(cmd.OutOrStdout(), resources, cliui.WorkspaceResourcesOptions{
WorkspaceName: workspaceName,
// Since agent's haven't connected yet, hiding this makes more sense.
HideAgentState: true,
Title: "Workspace Preview",
parameters, err := prepWorkspaceBuild(cmd, client, prepWorkspaceBuildArgs{
Template: template,
ExistingParams: []codersdk.Parameter{},
ParameterFile: parameterFile,
NewWorkspaceName: workspaceName,
})
if err != nil {
return err
Expand All @@ -214,6 +138,7 @@ func create() *cobra.Command {
return err
}

after := time.Now()
workspace, err := client.CreateWorkspace(cmd.Context(), organization.ID, codersdk.CreateWorkspaceRequest{
TemplateID: template.ID,
Name: workspaceName,
Expand Down Expand Up @@ -242,3 +167,118 @@ func create() *cobra.Command {
cliflag.DurationVarP(cmd.Flags(), &stopAfter, "stop-after", "", "CODER_WORKSPACE_STOP_AFTER", 8*time.Hour, "Specify a duration after which the workspace should shut down (e.g. 8h).")
return cmd
}

type prepWorkspaceBuildArgs struct {
Template codersdk.Template
ExistingParams []codersdk.Parameter
ParameterFile string
NewWorkspaceName string
}

// prepWorkspaceBuild will ensure a workspace build will succeed on the latest template version.
// Any missing params will be prompted to the user.
func prepWorkspaceBuild(cmd *cobra.Command, client *codersdk.Client, args prepWorkspaceBuildArgs) ([]codersdk.CreateParameterRequest, error) {
ctx := cmd.Context()
templateVersion, err := client.TemplateVersion(ctx, args.Template.ActiveVersionID)
if err != nil {
return nil, err
}
parameterSchemas, err := client.TemplateVersionSchema(ctx, templateVersion.ID)
if err != nil {
return nil, err
}

// parameterMapFromFile can be nil if parameter file is not specified
var parameterMapFromFile map[string]string
useParamFile := false
if args.ParameterFile != "" {
useParamFile = true
_, _ = fmt.Fprintln(cmd.OutOrStdout(), cliui.Styles.Paragraph.Render("Attempting to read the variables from the parameter file.")+"\r\n")
parameterMapFromFile, err = createParameterMapFromFile(args.ParameterFile)
if err != nil {
return nil, err
}
}
disclaimerPrinted := false
parameters := make([]codersdk.CreateParameterRequest, 0)
PromptParamLoop:
for _, parameterSchema := range parameterSchemas {
if !parameterSchema.AllowOverrideSource {
continue
}
if !disclaimerPrinted {
_, _ = fmt.Fprintln(cmd.OutOrStdout(), cliui.Styles.Paragraph.Render("This template has customizable parameters. Values can be changed after create, but may have unintended side effects (like data loss).")+"\r\n")
disclaimerPrinted = true
}

// Param file is all or nothing
if !useParamFile {
for _, e := range args.ExistingParams {
if e.Name == parameterSchema.Name {
// If the param already exists, we do not need to prompt it again.
// The workspace scope will reuse params for each build.
continue PromptParamLoop
}
}
}

parameterValue, err := getParameterValueFromMapOrInput(cmd, parameterMapFromFile, parameterSchema)
if err != nil {
return nil, err
}

parameters = append(parameters, codersdk.CreateParameterRequest{
Name: parameterSchema.Name,
SourceValue: parameterValue,
SourceScheme: codersdk.ParameterSourceSchemeData,
DestinationScheme: parameterSchema.DefaultDestinationScheme,
})
}
_, _ = fmt.Fprintln(cmd.OutOrStdout())

// Run a dry-run with the given parameters to check correctness
after := time.Now()
dryRun, err := client.CreateTemplateVersionDryRun(cmd.Context(), templateVersion.ID, codersdk.CreateTemplateVersionDryRunRequest{
WorkspaceName: args.NewWorkspaceName,
ParameterValues: parameters,
})
if err != nil {
return nil, xerrors.Errorf("begin workspace dry-run: %w", err)
}
_, _ = fmt.Fprintln(cmd.OutOrStdout(), "Planning workspace...")
err = cliui.ProvisionerJob(cmd.Context(), cmd.OutOrStdout(), cliui.ProvisionerJobOptions{
Fetch: func() (codersdk.ProvisionerJob, error) {
return client.TemplateVersionDryRun(cmd.Context(), templateVersion.ID, dryRun.ID)
},
Cancel: func() error {
return client.CancelTemplateVersionDryRun(cmd.Context(), templateVersion.ID, dryRun.ID)
},
Logs: func() (<-chan codersdk.ProvisionerJobLog, error) {
return client.TemplateVersionDryRunLogsAfter(cmd.Context(), templateVersion.ID, dryRun.ID, after)
},
// Don't show log output for the dry-run unless there's an error.
Silent: true,
})
if err != nil {
// TODO (Dean): reprompt for parameter values if we deem it to
// be a validation error
return nil, xerrors.Errorf("dry-run workspace: %w", err)
}

resources, err := client.TemplateVersionDryRunResources(cmd.Context(), templateVersion.ID, dryRun.ID)
if err != nil {
return nil, xerrors.Errorf("get workspace dry-run resources: %w", err)
}

err = cliui.WorkspaceResources(cmd.OutOrStdout(), resources, cliui.WorkspaceResourcesOptions{
WorkspaceName: args.NewWorkspaceName,
// Since agents haven't connected yet, hiding this makes more sense.
HideAgentState: true,
Title: "Workspace Preview",
})
if err != nil {
return nil, err
}

return parameters, nil
}
2 changes: 1 addition & 1 deletion cli/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
)

// nolint
func delete() *cobra.Command {
func deleteWorkspace() *cobra.Command {
cmd := &cobra.Command{
Annotations: workspaceCommand,
Use: "delete <workspace>",
Expand Down
2 changes: 1 addition & 1 deletion cli/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func Root() *cobra.Command {
cmd.AddCommand(
configSSH(),
create(),
delete(),
deleteWorkspace(),
dotfiles(),
gitssh(),
list(),
Expand Down
34 changes: 32 additions & 2 deletions cli/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,17 @@ import (

"github.com/spf13/cobra"

"github.com/coder/coder/cli/cliflag"
"github.com/coder/coder/codersdk"
)

func update() *cobra.Command {
return &cobra.Command{
var (
parameterFile string
alwaysPrompt bool
)

cmd := &cobra.Command{
Annotations: workspaceCommand,
Use: "update",
Short: "Update a workspace to the latest template version",
Expand All @@ -23,18 +29,38 @@ func update() *cobra.Command {
if err != nil {
return err
}
if !workspace.Outdated {
if !workspace.Outdated && !alwaysPrompt {
_, _ = fmt.Printf("Workspace isn't outdated!\n")
return nil
}
template, err := client.Template(cmd.Context(), workspace.TemplateID)
if err != nil {
return nil
}

var existingParams []codersdk.Parameter
if !alwaysPrompt {
existingParams, err = client.Parameters(cmd.Context(), codersdk.ParameterWorkspace, workspace.ID)
if err != nil {
return nil
}
}

parameters, err := prepWorkspaceBuild(cmd, client, prepWorkspaceBuildArgs{
Template: template,
ExistingParams: existingParams,
ParameterFile: parameterFile,
NewWorkspaceName: workspace.Name,
})
Comment on lines +49 to +54
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if this always ran and had an argument to prompt for missing params or not instead of only running if they prompted for params. We'd be able to always dry-run this way

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean instead of passing "ExistingParams", detect that inside the prepWorkspaceBuild? Cause if so, I do want to do this. Templates use similar logic, so I want to refactor the create/update of both workspaces/templates to use the same code.

I was going to take a look in another PR.

if err != nil {
return nil
}

before := time.Now()
build, err := client.CreateWorkspaceBuild(cmd.Context(), workspace.ID, codersdk.CreateWorkspaceBuildRequest{
TemplateVersionID: template.ActiveVersionID,
Transition: workspace.LatestBuild.Transition,
ParameterValues: parameters,
})
if err != nil {
return err
Expand All @@ -53,4 +79,8 @@ func update() *cobra.Command {
return nil
},
}

cmd.Flags().BoolVar(&alwaysPrompt, "always-prompt", false, "Always prompt all parameters. Does not pull parameter values from existing workspace")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--reprompt sounds better. Or we could keep it called --always-prompt and ask the user if they want to edit their parameters using a y/n prompt.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use --always-prompt and a y/n prompt?

I am not sold on always-prompt either, was unsure on what exactly to use 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant --always-prompt, but if not specified ask them if they want to change values with a y/n prompt

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh. I was trying to keep the flow quick.

I think we need to invest time into thinking about our UX in general. Idk if we should do coder edit or coder configure to adjust params, but we probably need some command UX to manage params in general.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add this prompt though if always-prompt is omitted 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our y/n prompt only returns yes or error. We need to modify it to accept a proper y/n if we want to do something other than abort on no.

cliflag.StringVarP(cmd.Flags(), &parameterFile, "parameter-file", "", "CODER_PARAMETER_FILE", "", "Specify a file path with parameter values.")
return cmd
}
37 changes: 37 additions & 0 deletions coderd/workspacebuilds.go
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,43 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
// This must happen in a transaction to ensure history can be inserted, and
// the prior history can update it's "after" column to point at the new.
err = api.Database.InTx(func(db database.Store) error {
existing, err := db.ParameterValues(r.Context(), database.ParameterValuesParams{
Scopes: []database.ParameterScope{database.ParameterScopeWorkspace},
ScopeIds: []uuid.UUID{workspace.ID},
})
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
return xerrors.Errorf("Fetch previous parameters: %w", err)
}

// Write/Update any new params
now := database.Now()
for _, param := range createBuild.ParameterValues {
for _, exists := range existing {
// If the param exists, delete the old param before inserting the new one
if exists.Name == param.Name {
err = db.DeleteParameterValueByID(r.Context(), exists.ID)
if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
return xerrors.Errorf("Failed to delete old param %q: %w", exists.Name, err)
}
}
}

_, err = db.InsertParameterValue(r.Context(), database.InsertParameterValueParams{
ID: uuid.New(),
Name: param.Name,
CreatedAt: now,
UpdatedAt: now,
Scope: database.ParameterScopeWorkspace,
ScopeID: workspace.ID,
SourceScheme: database.ParameterSourceScheme(param.SourceScheme),
SourceValue: param.SourceValue,
DestinationScheme: database.ParameterDestinationScheme(param.DestinationScheme),
})
if err != nil {
return xerrors.Errorf("insert parameter value: %w", err)
}
}

workspaceBuildID := uuid.New()
input, err := json.Marshal(workspaceProvisionJob{
WorkspaceBuildID: workspaceBuildID,
Expand Down
4 changes: 4 additions & 0 deletions codersdk/workspaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ type CreateWorkspaceBuildRequest struct {
Transition WorkspaceTransition `json:"transition" validate:"oneof=create start stop delete,required"`
DryRun bool `json:"dry_run,omitempty"`
ProvisionerState []byte `json:"state,omitempty"`
// ParameterValues are optional. It will write params to the 'workspace' scope.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should explain the behavior that it doesn't delete the old values, but any present values will overwrite any existing values with the same name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true... I wonder if I should delete the unused params 🤔. I can add this in another PR and test it more

// This will overwrite any existing parameters with the same name.
// This will not delete old params not included in this list.
ParameterValues []CreateParameterRequest `json:"parameter_values,omitempty"`
}

type WorkspaceOptions struct {
Expand Down
Loading