Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update validations #341

Merged
merged 1 commit into from Mar 4, 2022
Merged

Update validations #341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 29 additions & 3 deletions admin_organization.go
View file
Open in desktop
Expand Up @@ -76,9 +76,7 @@ type AdminOrganizationList struct {
// https://www.terraform.io/docs/cloud/api/admin/organizations.html#available-related-resources
type AdminOrgIncludeOpt string

const (
AdminOrgOwners AdminOrgIncludeOpt = "owners"
)
const AdminOrgOwners AdminOrgIncludeOpt = "owners"

// AdminOrganizationListOptions represents the options for listing organizations via Admin API.
type AdminOrganizationListOptions struct {
Expand All @@ -103,6 +101,9 @@ type AdminOrganizationID struct {

// List all the organizations visible to the current user.
func (s *adminOrganizations) List(ctx context.Context, options *AdminOrganizationListOptions) (*AdminOrganizationList, error) {
if err := options.valid(); err != nil {
return nil, err
}
u := "admin/organizations"
req, err := s.client.newRequest("GET", u, options)
if err != nil {
Expand Down Expand Up @@ -225,3 +226,28 @@ func (s *adminOrganizations) Delete(ctx context.Context, organization string) er

return s.client.do(ctx, req, nil)
}

func (o *AdminOrganizationListOptions) valid() error {
if o == nil {
return nil // nothing to validate
}

if err := validateAdminOrgIncludeParams(o.Include); err != nil {
return err
}

return nil
}

func validateAdminOrgIncludeParams(params []AdminOrgIncludeOpt) error {
for _, p := range params {
switch p {
case AdminOrgOwners:
// do nothing
default:
return ErrInvalidIncludeValue
}
}

return nil
}
80 changes: 54 additions & 26 deletions admin_run.go
View file
Open in desktop
Expand Up @@ -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 {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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 :(

// 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
}
17 changes: 17 additions & 0 deletions admin_run_integration_test.go
View file
Open in desktop
Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Copy link
Collaborator Author

@uturunku1 uturunku1 Mar 4, 2022 •
edited

Choose a reason for hiding this comment

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

This test and all the ones for TestAdminRuns_AdminRunsListOptions_valid don't really need to be part of integration test suite. We are just testing helper functions and it'd be nice if external contributors could have access to running these tests. I'd create a unit test file, like I did with tfe_test.go, but then suddenly I end up with 2 new unit test files, suddenly I feel like they should have their own directory to live..., suddenly I have the urge to reorganize other files. But this is not the time to reorg things.
The subject of reorganizing the structure of this project for upcoming 1.0 release has been discussed (because we know is not scalable to continue adding new files to root directory). We decided to postpone. It would involve too many breaking changes on top of the ones we are already bringing. But I really hope that we can find a solution to the "how to continue adding files to this project" topic.

opts := AdminRunsListOptions{
RunStatus: "pending, ",
}

err := opts.valid()
assert.NoError(t, err)
})
}

func TestAdminRun_ForceCancel_Marshal(t *testing.T) {
Expand Down
23 changes: 14 additions & 9 deletions admin_setting_smtp.go
View file
Open in desktop
Expand Up @@ -30,12 +30,6 @@ const (
SMTPAuthLogin SMTPAuthType = "login"
)

var validSMTPAuthType = map[SMTPAuthType]struct{}{
SMTPAuthNone: {},
SMTPAuthPlain: {},
SMTPAuthLogin: {},
}

// AdminSMTPSetting represents a the SMTP settings in Terraform Enterprise.
type AdminSMTPSetting struct {
ID string `jsonapi:"primary,smtp-settings"`
Expand Down Expand Up @@ -82,6 +76,7 @@ func (a *adminSMTPSettings) Update(ctx context.Context, options AdminSMTPSetting
if err := options.valid(); err != nil {
return nil, err
}

req, err := a.client.newRequest("PATCH", "admin/smtp-settings", &options)
if err != nil {
return nil, err
Expand All @@ -98,11 +93,21 @@ func (a *adminSMTPSettings) Update(ctx context.Context, options AdminSMTPSetting

func (o AdminSMTPSettingsUpdateOptions) valid() error {
if validString((*string)(o.Auth)) {
_, isValidType := validSMTPAuthType[*o.Auth]
if !isValidType {
return ErrInvalidSMTPAuth
if err := validateAdminSettingSMTPAuth(*o.Auth); err != nil {
return err
}
}

return nil
}

func validateAdminSettingSMTPAuth(authVal SMTPAuthType) error {
switch authVal {
case SMTPAuthNone, SMTPAuthPlain, SMTPAuthLogin:
// do nothing
default:
return ErrInvalidSMTPAuth
}

return nil
}
16 changes: 13 additions & 3 deletions admin_setting_smtp_integration_test.go
View file
Open in desktop
Expand Up @@ -35,16 +35,17 @@ func TestAdminSettings_SMTP_Update(t *testing.T) {
client := testClient(t)
ctx := context.Background()

enabled := false
enabled := true
disabled := false

t.Run("with Auth option defined", func(t *testing.T) {
smtpSettings, err := client.Admin.Settings.SMTP.Update(ctx, AdminSMTPSettingsUpdateOptions{
Enabled: Bool(enabled),
Enabled: Bool(disabled),
Auth: SMTPAuthValue(SMTPAuthNone),
})

require.NoError(t, err)
assert.Equal(t, enabled, smtpSettings.Enabled)
assert.Equal(t, disabled, smtpSettings.Enabled)
})
t.Run("with no Auth option", func(t *testing.T) {
smtpSettings, err := client.Admin.Settings.SMTP.Update(ctx, AdminSMTPSettingsUpdateOptions{
Expand All @@ -55,4 +56,13 @@ func TestAdminSettings_SMTP_Update(t *testing.T) {
assert.Equal(t, SMTPAuthNone, smtpSettings.Auth)
assert.Equal(t, enabled, smtpSettings.Enabled)
})
t.Run("with invalid Auth option", func(t *testing.T) {
var SMTPAuthPlained SMTPAuthType = "plained"
_, err := client.Admin.Settings.SMTP.Update(ctx, AdminSMTPSettingsUpdateOptions{
Enabled: Bool(enabled),
Auth: &SMTPAuthPlained,
})

assert.Equal(t, err, ErrInvalidSMTPAuth)
})
}
32 changes: 29 additions & 3 deletions admin_user.go
View file
Open in desktop
Expand Up @@ -68,9 +68,7 @@ type AdminUserList struct {
// https://www.terraform.io/docs/cloud/api/admin/users.html#available-related-resources
type AdminUserIncludeOpt string

const (
AdminUserOrgs AdminUserIncludeOpt = "organizations"
)
const AdminUserOrgs AdminUserIncludeOpt = "organizations"

// AdminUserListOptions represents the options for listing users.
// https://www.terraform.io/docs/cloud/api/admin/users.html#query-parameters
Expand All @@ -93,6 +91,10 @@ type AdminUserListOptions struct {

// List all user accounts in the Terraform Enterprise installation
func (a *adminUsers) List(ctx context.Context, options *AdminUserListOptions) (*AdminUserList, error) {
if err := options.valid(); err != nil {
return nil, err
}

u := "admin/users"
req, err := a.client.newRequest("GET", u, options)
if err != nil {
Expand Down Expand Up @@ -228,3 +230,27 @@ func (a *adminUsers) Disable2FA(ctx context.Context, userID string) (*AdminUser,

return au, nil
}

func (o *AdminUserListOptions) valid() error {
if o == nil {
return nil // nothing to validate
}

if err := validateAdminUserIncludeParams(o.Include); err != nil {
return err
}

return nil
}

func validateAdminUserIncludeParams(params []AdminUserIncludeOpt) error {
for _, p := range params {
switch p {
case AdminUserOrgs:
// do nothing
default:
return ErrInvalidIncludeValue
}
}
return nil
}
29 changes: 29 additions & 0 deletions admin_workspace.go
View file
Open in desktop
Expand Up @@ -76,6 +76,10 @@ type AdminWorkspaceList struct {

// List all the workspaces within a workspace.
func (s *adminWorkspaces) List(ctx context.Context, options *AdminWorkspaceListOptions) (*AdminWorkspaceList, error) {
if err := options.valid(); err != nil {
return nil, err
}

u := "admin/workspaces"
req, err := s.client.newRequest("GET", u, options)
if err != nil {
Expand Down Expand Up @@ -126,3 +130,28 @@ func (s *adminWorkspaces) Delete(ctx context.Context, workspaceID string) error

return s.client.do(ctx, req, nil)
}

func (o *AdminWorkspaceListOptions) valid() error {
if o == nil {
return nil // nothing to validate
}

if err := validateAdminWSIncludeParams(o.Include); err != nil {
return err
}

return nil
}

func validateAdminWSIncludeParams(params []AdminWorkspaceIncludeOpt) error {
for _, p := range params {
switch p {
case AdminWorkspaceOrg, AdminWorkspaceCurrentRun, AdminWorkspaceOrgOwners:
// do nothing
default:
return ErrInvalidIncludeValue
}
}

return nil
}