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
Update validations #341
Update validations #341
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,39 +116,67 @@ func (s *adminRuns) ForceCancel(ctx context.Context, runID string, options Admin | |
} | ||
|
||
func (o *AdminRunsListOptions) valid() error { | ||
if o == nil { // no need to validate fields | ||
if o == nil { // nothing to validate | ||
return nil | ||
} | ||
if validString(&o.RunStatus) { | ||
validRunStatus := map[string]int{ | ||
string(RunApplied): 1, | ||
string(RunApplyQueued): 1, | ||
string(RunApplying): 1, | ||
string(RunCanceled): 1, | ||
string(RunConfirmed): 1, | ||
string(RunCostEstimated): 1, | ||
string(RunCostEstimating): 1, | ||
string(RunDiscarded): 1, | ||
string(RunErrored): 1, | ||
string(RunPending): 1, | ||
string(RunPlanQueued): 1, | ||
string(RunPlanned): 1, | ||
string(RunPlannedAndFinished): 1, | ||
string(RunPlanning): 1, | ||
string(RunPolicyChecked): 1, | ||
string(RunPolicyChecking): 1, | ||
string(RunPolicyOverride): 1, | ||
string(RunPolicySoftFailed): 1, | ||
} | ||
runStatus := strings.Split(o.RunStatus, ",") | ||
|
||
if err := validateAdminRunFilterParams(o.RunStatus); err != nil { | ||
return err | ||
} | ||
|
||
if err := validateAdminRunIncludeParams(o.Include); err != nil { | ||
return err | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func validateAdminRunFilterParams(runStatus string) error { | ||
// For the platform, an invalid filter value is a semantically understood query that returns an empty set, no error, no warning. But for go-tfe, an invalid value is good enough reason to error prior to a network call to the platform: | ||
if validString(&runStatus) { | ||
sanitizedRunstatus := strings.TrimSpace(runStatus) | ||
runStatuses := strings.Split(sanitizedRunstatus, ",") | ||
// iterate over our statuses, and ensure it is valid. | ||
for _, status := range runStatus { | ||
if _, present := validRunStatus[status]; !present { | ||
return fmt.Errorf("invalid value %s for run status", status) | ||
for _, status := range runStatuses { | ||
switch status { | ||
case string(RunApplied), | ||
string(RunApplyQueued), | ||
string(RunApplying), | ||
string(RunCanceled), | ||
string(RunConfirmed), | ||
string(RunCostEstimate), | ||
string(RunCostEstimating), | ||
string(RunDiscarded), | ||
string(RunErrored), | ||
string(RunPending), | ||
string(RunPlanQueued), | ||
string(RunPlanned), | ||
string(RunPlannedAndFinished), | ||
string(RunPlanning), | ||
string(RunPolicyChecked), | ||
string(RunPolicyChecking), | ||
string(RunPolicyOverride), | ||
string(RunPolicySoftFailed), | ||
"": | ||
// do nothing | ||
default: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should allow an empty string because one could be split if there was a trailing comma. The API would accept a trailing comma, I think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call! It could also have trailing space. OK! I am writing a new test for this. |
||
return fmt.Errorf(`invalid value "%s" for run status`, status) | ||
} | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func validateAdminRunIncludeParams(params []AdminRunIncludeOpt) error { | ||
for _, p := range params { | ||
switch p { | ||
case AdminRunWorkspace, AdminRunWorkspaceOrg, AdminRunWorkspaceOrgOwners: | ||
// do nothing | ||
default: | ||
return ErrInvalidIncludeValue | ||
} | ||
} | ||
|
||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,14 @@ func TestAdminRuns_List(t *testing.T) { | |
assert.NotEmpty(t, rl.Items[0].Workspace.Organization.Name) | ||
}) | ||
|
||
t.Run("with invalid Include option", func(t *testing.T) { | ||
_, err := client.Admin.Runs.List(ctx, &AdminRunsListOptions{ | ||
Include: []AdminRunIncludeOpt{"workpsace"}, | ||
}) | ||
|
||
assert.Equal(t, err, ErrInvalidIncludeValue) | ||
}) | ||
|
||
t.Run("with RunStatus.pending filter", func(t *testing.T) { | ||
r1, err := client.Runs.Read(ctx, rTest1.ID) | ||
assert.NoError(t, err) | ||
|
@@ -244,6 +252,15 @@ func TestAdminRuns_AdminRunsListOptions_valid(t *testing.T) { | |
err := opts.valid() | ||
assert.Error(t, err) | ||
}) | ||
|
||
t.Run("has trailing comma and trailing space", func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test and all the ones for |
||
opts := AdminRunsListOptions{ | ||
RunStatus: "pending, ", | ||
} | ||
|
||
err := opts.valid() | ||
assert.NoError(t, err) | ||
}) | ||
} | ||
|
||
func TestAdminRun_ForceCancel_Marshal(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not possible to define RunStatus as type RunStatus on AdminRunsListOptions so you can avoid the string type here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed! I'll make it into a typed string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, converting RunStatus filter option from a string into a typed string lis going to a larger deal than expected. Filter options is a comma-separated list https://www.terraform.io/cloud-docs/api-docs/admin/runs#query-parameters just like Include options. So I’d have to apply the same types of changes I made for include field: make it into a slice of typed strings, then convert it back into string of comma-separated values when I call client.newRequest . Then I would have to apply the same changes to all filter options not just RunStatus to create consistency in this behavior for all filter options :(