Skip to content

fix: prompt when parameter options are incompatible #9247

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 13 commits into from
Aug 23, 2023
28 changes: 28 additions & 0 deletions cli/parameterresolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,10 @@ next:
continue // immutables should not be passed to consecutive builds
}

if len(tvp.Options) > 0 && !isValidTemplateParameterOption(buildParameter, tvp.Options) {
continue // do not propagate invalid options
}

for i, r := range resolved {
if r.Name == buildParameter.Name {
resolved[i].Value = buildParameter.Value
Expand Down Expand Up @@ -180,10 +184,12 @@ func (pr *ParameterResolver) resolveWithInput(resolved []codersdk.WorkspaceBuild
// Parameter has not been resolved yet, so CLI needs to determine if user should input it.

firstTimeUse := pr.isFirstTimeUse(tvp.Name)
promptParameterOption := pr.isLastBuildParameterInvalidOption(tvp)

if (tvp.Ephemeral && pr.promptBuildOptions) ||
(action == WorkspaceCreate && tvp.Required) ||
(action == WorkspaceCreate && !tvp.Ephemeral) ||
(action == WorkspaceUpdate && promptParameterOption) ||
(action == WorkspaceUpdate && tvp.Mutable && tvp.Required) ||
(action == WorkspaceUpdate && !tvp.Mutable && firstTimeUse) ||
(action == WorkspaceUpdate && tvp.Mutable && !tvp.Ephemeral && pr.promptRichParameters) {
Expand All @@ -207,6 +213,19 @@ func (pr *ParameterResolver) isFirstTimeUse(parameterName string) bool {
return findWorkspaceBuildParameter(parameterName, pr.lastBuildParameters) == nil
}

func (pr *ParameterResolver) isLastBuildParameterInvalidOption(templateVersionParameter codersdk.TemplateVersionParameter) bool {
if len(templateVersionParameter.Options) == 0 {
return false
}

for _, buildParameter := range pr.lastBuildParameters {
if buildParameter.Name == templateVersionParameter.Name {
return !isValidTemplateParameterOption(buildParameter, templateVersionParameter.Options)
}
}
return false
}

func findTemplateVersionParameter(workspaceBuildParameter codersdk.WorkspaceBuildParameter, templateVersionParameters []codersdk.TemplateVersionParameter) *codersdk.TemplateVersionParameter {
for _, tvp := range templateVersionParameters {
if tvp.Name == workspaceBuildParameter.Name {
Expand All @@ -224,3 +243,12 @@ func findWorkspaceBuildParameter(parameterName string, params []codersdk.Workspa
}
return nil
}

func isValidTemplateParameterOption(buildParameter codersdk.WorkspaceBuildParameter, options []codersdk.TemplateVersionParameterOption) bool {
for _, opt := range options {
if opt.Value == buildParameter.Value {
return true
}
}
return false
}
2 changes: 2 additions & 0 deletions cli/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -981,6 +981,8 @@ func (p *prettyErrorFormatter) format(err error) {
msg = sdkError.Message
if sdkError.Helper != "" {
msg = msg + "\n" + sdkError.Helper
} else if sdkError.Detail != "" {
Copy link
Member Author

Choose a reason for hiding this comment

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

for reviewers:

Message: "Unable to validate parameter X"
Detail: "parameter value must match one of options"

^ this should be consistent with site UI error

msg = msg + "\n" + sdkError.Detail
}
// The SDK error is usually good enough, and we don't want to overwhelm
// the user with output.
Expand Down
126 changes: 126 additions & 0 deletions cli/update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -593,12 +593,138 @@ func TestUpdateValidateRichParameters(t *testing.T) {
assert.NoError(t, err)
}()

pty.ExpectMatch("Planning workspace...")
<-doneChan
})

t.Run("ParameterOptionChanged", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test case where the chosen template parameter disappears completely?

e.g. before: { 1st, 2nd, 3rd }, after: { 1st, 3rd, 4th }

Copy link
Member Author

Choose a reason for hiding this comment

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

Added!

t.Parallel()

// Create template and workspace
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
user := coderdtest.CreateFirstUser(t, client)

templateParameters := []*proto.RichParameter{
{Name: stringParameterName, Type: "string", Mutable: true, Required: true, Options: []*proto.RichParameterOption{
{Name: "First option", Description: "This is first option", Value: "1st"},
{Name: "Second option", Description: "This is second option", Value: "2nd"},
{Name: "Third option", Description: "This is third option", Value: "3rd"},
}},
}
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, prepareEchoResponses(templateParameters))
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)

inv, root := clitest.New(t, "create", "my-workspace", "--yes", "--template", template.Name, "--parameter", fmt.Sprintf("%s=%s", stringParameterName, "2nd"))
clitest.SetupConfig(t, client, root)
err := inv.Run()
require.NoError(t, err)

// Update template
updatedTemplateParameters := []*proto.RichParameter{
{Name: stringParameterName, Type: "string", Mutable: true, Required: true, Options: []*proto.RichParameterOption{
{Name: "first_option", Description: "This is first option", Value: "1"},
{Name: "second_option", Description: "This is second option", Value: "2"},
{Name: "third_option", Description: "This is third option", Value: "3"},
}},
}

updatedVersion := coderdtest.UpdateTemplateVersion(t, client, user.OrganizationID, prepareEchoResponses(updatedTemplateParameters), template.ID)
coderdtest.AwaitTemplateVersionJob(t, client, updatedVersion.ID)
err = client.UpdateActiveTemplateVersion(context.Background(), template.ID, codersdk.UpdateActiveTemplateVersion{
ID: updatedVersion.ID,
})
require.NoError(t, err)

// Update the workspace
inv, root = clitest.New(t, "update", "my-workspace")
clitest.SetupConfig(t, client, root)
doneChan := make(chan struct{})
pty := ptytest.New(t).Attach(inv)
go func() {
defer close(doneChan)
err := inv.Run()
assert.NoError(t, err)
}()

matches := []string{
stringParameterName, "second_option",
"Planning workspace...", "",
}
for i := 0; i < len(matches); i += 2 {
match := matches[i]
value := matches[i+1]
pty.ExpectMatch(match)

if value != "" {
pty.WriteLine(value)
}
}
<-doneChan
})

t.Run("ParameterOptionDisappeared", func(t *testing.T) {
t.Parallel()

// Create template and workspace
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
user := coderdtest.CreateFirstUser(t, client)

templateParameters := []*proto.RichParameter{
{Name: stringParameterName, Type: "string", Mutable: true, Required: true, Options: []*proto.RichParameterOption{
{Name: "First option", Description: "This is first option", Value: "1st"},
{Name: "Second option", Description: "This is second option", Value: "2nd"},
{Name: "Third option", Description: "This is third option", Value: "3rd"},
}},
}
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, prepareEchoResponses(templateParameters))
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID)

inv, root := clitest.New(t, "create", "my-workspace", "--yes", "--template", template.Name, "--parameter", fmt.Sprintf("%s=%s", stringParameterName, "2nd"))
clitest.SetupConfig(t, client, root)
err := inv.Run()
require.NoError(t, err)

// Update template - 2nd option disappeared, 4th option added
updatedTemplateParameters := []*proto.RichParameter{
{Name: stringParameterName, Type: "string", Mutable: true, Required: true, Options: []*proto.RichParameterOption{
{Name: "First option", Description: "This is first option", Value: "1st"},
{Name: "Third option", Description: "This is third option", Value: "3rd"},
{Name: "Fourth option", Description: "This is fourth option", Value: "4th"},
}},
}

updatedVersion := coderdtest.UpdateTemplateVersion(t, client, user.OrganizationID, prepareEchoResponses(updatedTemplateParameters), template.ID)
coderdtest.AwaitTemplateVersionJob(t, client, updatedVersion.ID)
err = client.UpdateActiveTemplateVersion(context.Background(), template.ID, codersdk.UpdateActiveTemplateVersion{
ID: updatedVersion.ID,
})
require.NoError(t, err)

// Update the workspace
inv, root = clitest.New(t, "update", "my-workspace")
clitest.SetupConfig(t, client, root)
doneChan := make(chan struct{})
pty := ptytest.New(t).Attach(inv)
go func() {
defer close(doneChan)
err := inv.Run()
assert.NoError(t, err)
}()

matches := []string{
stringParameterName, "Third option",
"Planning workspace...", "",
}
for i := 0; i < len(matches); i += 2 {
match := matches[i]
value := matches[i+1]
pty.ExpectMatch(match)

if value != "" {
pty.WriteLine(value)
}
}
<-doneChan
})
Expand Down
3 changes: 0 additions & 3 deletions coderd/apidoc/docs.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 3 additions & 6 deletions coderd/apidoc/swagger.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion coderd/wsbuilder/wsbuilder.go
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ func (b *Builder) getParameters() (names, values []string, err error) {
// At this point, we've queried all the data we need from the database,
// so the only errors are problems with the request (missing data, failed
// validation, immutable parameters, etc.)
return nil, nil, BuildError{http.StatusBadRequest, err.Error(), err}
return nil, nil, BuildError{http.StatusBadRequest, fmt.Sprintf("Unable to validate parameter %q", templateVersionParameter.Name), err}
}
names = append(names, templateVersionParameter.Name)
values = append(values, value)
Expand Down
3 changes: 1 addition & 2 deletions codersdk/provisionerdaemons.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ const (
type JobErrorCode string

const (
MissingTemplateParameter JobErrorCode = "MISSING_TEMPLATE_PARAMETER"
Copy link
Member Author

Choose a reason for hiding this comment

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

for reviewers: used by old legacy parameters (do not exist anymore)

RequiredTemplateVariables JobErrorCode = "REQUIRED_TEMPLATE_VARIABLES"
)

Expand All @@ -81,7 +80,7 @@ type ProvisionerJob struct {
CompletedAt *time.Time `json:"completed_at,omitempty" format:"date-time"`
CanceledAt *time.Time `json:"canceled_at,omitempty" format:"date-time"`
Error string `json:"error,omitempty"`
ErrorCode JobErrorCode `json:"error_code,omitempty" enums:"MISSING_TEMPLATE_PARAMETER,REQUIRED_TEMPLATE_VARIABLES"`
ErrorCode JobErrorCode `json:"error_code,omitempty" enums:"REQUIRED_TEMPLATE_VARIABLES"`
Status ProvisionerJobStatus `json:"status" enums:"pending,running,succeeded,canceling,canceled,failed"`
WorkerID *uuid.UUID `json:"worker_id,omitempty" format:"uuid"`
FileID uuid.UUID `json:"file_id" format:"uuid"`
Expand Down
11 changes: 5 additions & 6 deletions docs/api/builds.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 6 additions & 8 deletions docs/api/schemas.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading