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

Remove dereference to pointer for include and query fields for ListOption structs #309

Merged
merged 1 commit into from Feb 14, 2022
Merged

Remove dereference to pointer for include and query fields for ListOption structs #309

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
4 changes: 2 additions & 2 deletions admin_organization.go
View file
Open in desktop
Expand Up @@ -86,9 +86,9 @@ type AdminOrganizationListOptions struct {

// A query string used to filter organizations.
// Any organizations with a name or notification email partially matching this value will be returned.
Query *string `url:"q,omitempty"`
Query string `url:"q,omitempty"`

Include *[]AdminOrgIncludeOps `url:"include,omitempty"`
Include []AdminOrgIncludeOps `url:"include,omitempty"`
}

// AdminOrganizationListModuleConsumersOptions represents the options for listing organization module consumers through the Admin API
Expand Down
6 changes: 3 additions & 3 deletions admin_organization_integration_test.go
View file
Open in desktop
Expand Up @@ -37,7 +37,7 @@ func TestAdminOrganizations_List(t *testing.T) {
defer orgTestCleanup()

adminOrgList, err := client.Admin.Organizations.List(ctx, &AdminOrganizationListOptions{
Query: &org.Name,
Query: org.Name,
})
assert.NoError(t, err)
assert.Equal(t, true, adminOrgItemsContainsName(adminOrgList.Items, org.Name))
Expand All @@ -49,7 +49,7 @@ func TestAdminOrganizations_List(t *testing.T) {
randomName := "random-org-name"

adminOrgList, err := client.Admin.Organizations.List(ctx, &AdminOrganizationListOptions{
Query: &randomName,
Query: randomName,
})
assert.NoError(t, err)
assert.Equal(t, false, adminOrgItemsContainsName(adminOrgList.Items, org.Name))
Expand All @@ -59,7 +59,7 @@ func TestAdminOrganizations_List(t *testing.T) {

t.Run("with owners included", func(t *testing.T) {
adminOrgList, err := client.Admin.Organizations.List(ctx, &AdminOrganizationListOptions{
Include: &([]AdminOrgIncludeOps{AdminOrgOwners}),
Include: []AdminOrgIncludeOps{AdminOrgOwners},
})
assert.NoError(t, err)

Expand Down
10 changes: 5 additions & 5 deletions admin_run.go
View file
Open in desktop
Expand Up @@ -62,9 +62,9 @@ const (
type AdminRunsListOptions struct {
ListOptions

RunStatus *string `url:"filter[status],omitempty"`
Query *string `url:"q,omitempty"`
Include *[]AdminRunIncludeOps `url:"include,omitempty"`
RunStatus string `url:"filter[status],omitempty"`
Query string `url:"q,omitempty"`
Include []AdminRunIncludeOps `url:"include,omitempty"`
}

// List all the runs of the terraform enterprise installation.
Expand Down Expand Up @@ -116,7 +116,7 @@ func (s *adminRuns) ForceCancel(ctx context.Context, runID string, options Admin

// Check that the field RunStatus has a valid string value
func (o AdminRunsListOptions) valid() error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make this into *AdminRunsListOptions?

Copy link
Collaborator

@uturunku1 uturunku1 Feb 14, 2022 •
edited

Choose a reason for hiding this comment

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

NVM. I was looking at other listOptions that call the valid() helper function (for example func (o StateVersionListOptions) valid() ) and none of them have the pointer type in the receiver method. I guess we could add them to all or just skip this step. It doesn't seem to make any difference to have it or to not have because at the end we are not checking if listOptions is valid but only if its fields are valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the most important thing here is consistency and I'm fine leaving it as is. Since valid() doesn't mutate values or ever really accept a complex struct (except maybe in workspace.go) I think a value receiver makes more sense.

Here's some nice reading: https://github.com/golang/go/wiki/CodeReviewComments#receiver-type

if validString(o.RunStatus) {
if validString(&o.RunStatus) {
validRunStatus := map[string]int{
string(RunApplied): 1,
string(RunApplyQueued): 1,
Expand All @@ -137,7 +137,7 @@ func (o AdminRunsListOptions) valid() error {
string(RunPolicyOverride): 1,
string(RunPolicySoftFailed): 1,
}
runStatus := strings.Split(*o.RunStatus, ",")
runStatus := strings.Split(o.RunStatus, ",")

// iterate over our statuses, and ensure it is valid.
for _, status := range runStatus {
Expand Down
18 changes: 9 additions & 9 deletions admin_run_integration_test.go
View file
Open in desktop
Expand Up @@ -69,7 +69,7 @@ func TestAdminRuns_List(t *testing.T) {

t.Run("with workspace included", func(t *testing.T) {
rl, err := client.Admin.Runs.List(ctx, &AdminRunsListOptions{
Include: &([]AdminRunIncludeOps{AdminRunWorkspace}),
Include: []AdminRunIncludeOps{AdminRunWorkspace},
})

assert.NoError(t, err)
Expand All @@ -81,7 +81,7 @@ func TestAdminRuns_List(t *testing.T) {

t.Run("with workspace.organization included", func(t *testing.T) {
rl, err := client.Admin.Runs.List(ctx, &AdminRunsListOptions{
Include: &([]AdminRunIncludeOps{AdminRunWorkspaceOrg}),
Include: []AdminRunIncludeOps{AdminRunWorkspaceOrg},
})

assert.NoError(t, err)
Expand All @@ -100,7 +100,7 @@ func TestAdminRuns_List(t *testing.T) {

// There should be pending Runs
rl, err := client.Admin.Runs.List(ctx, &AdminRunsListOptions{
RunStatus: String(string(RunPending)),
RunStatus: string(RunPending),
})
assert.NoError(t, err)
assert.NotEmpty(t, rl.Items)
Expand All @@ -114,15 +114,15 @@ func TestAdminRuns_List(t *testing.T) {
t.Run("with RunStatus.applied filter", func(t *testing.T) {
// There should be no applied Runs
rl, err := client.Admin.Runs.List(ctx, &AdminRunsListOptions{
RunStatus: String(string(RunApplied)),
RunStatus: string(RunApplied),
})
assert.NoError(t, err)
assert.Empty(t, rl.Items)
})

t.Run("with query", func(t *testing.T) {
rl, err := client.Admin.Runs.List(ctx, &AdminRunsListOptions{
Query: String(rTest1.ID),
Query: rTest1.ID,
})
assert.NoError(t, err)

Expand All @@ -131,7 +131,7 @@ func TestAdminRuns_List(t *testing.T) {
assert.Equal(t, adminRunItemsContainsID(rl.Items, rTest2.ID), false)

rl, err = client.Admin.Runs.List(ctx, &AdminRunsListOptions{
Query: String(rTest2.ID),
Query: rTest2.ID,
})
assert.NoError(t, err)

Expand Down Expand Up @@ -219,7 +219,7 @@ func TestAdminRuns_AdminRunsListOptions_valid(t *testing.T) {

t.Run("has valid status", func(t *testing.T) {
opts := AdminRunsListOptions{
RunStatus: String(string(RunPending)),
RunStatus: string(RunPending),
}

err := opts.valid()
Expand All @@ -228,7 +228,7 @@ func TestAdminRuns_AdminRunsListOptions_valid(t *testing.T) {

t.Run("has invalid status", func(t *testing.T) {
opts := AdminRunsListOptions{
RunStatus: String("random_status"),
RunStatus: "random_status",
}

err := opts.valid()
Expand All @@ -238,7 +238,7 @@ func TestAdminRuns_AdminRunsListOptions_valid(t *testing.T) {
t.Run("has invalid status, even with a valid one", func(t *testing.T) {
statuses := fmt.Sprintf("%s,%s", string(RunPending), "random_status")
opts := AdminRunsListOptions{
RunStatus: String(statuses),
RunStatus: statuses,
}

err := opts.valid()
Expand Down
4 changes: 2 additions & 2 deletions admin_terraform_version.go
View file
Open in desktop
Expand Up @@ -58,10 +58,10 @@ type AdminTerraformVersionsListOptions struct {
ListOptions

// A query string to find an exact version
Filter *string `url:"filter[version],omitempty"`
Filter string `url:"filter[version],omitempty"`

// A search query string to find all versions that match version substring
Search *string `url:"search[version],omitempty"`
Search string `url:"search[version],omitempty"`
}

// AdminTerraformVersionsList represents a list of terraform versions.
Expand Down
6 changes: 3 additions & 3 deletions admin_terraform_version_integration_test.go
View file
Open in desktop
Expand Up @@ -66,14 +66,14 @@ func TestAdminTerraformVersions_List(t *testing.T) {

t.Run("with filter query string", func(t *testing.T) {
tfList, err := client.Admin.TerraformVersions.List(ctx, &AdminTerraformVersionsListOptions{
Filter: String("1.0.4"),
Filter: "1.0.4",
})
require.NoError(t, err)
assert.Equal(t, 1, len(tfList.Items))

// Query for a Terraform version that does not exist
tfList, err = client.Admin.TerraformVersions.List(ctx, &AdminTerraformVersionsListOptions{
Filter: String("1000.1000.42"),
Filter: "1000.1000.42",
})
require.NoError(t, err)
assert.Empty(t, tfList.Items)
Expand All @@ -82,7 +82,7 @@ func TestAdminTerraformVersions_List(t *testing.T) {
t.Run("with search version query string", func(t *testing.T) {
searchVersion := "1.0"
tfList, err := client.Admin.TerraformVersions.List(ctx, &AdminTerraformVersionsListOptions{
Search: String(searchVersion),
Search: searchVersion,
})
require.NoError(t, err)
assert.NotEmpty(t, tfList.Items)
Expand Down
8 changes: 4 additions & 4 deletions admin_user.go
View file
Open in desktop
Expand Up @@ -78,15 +78,15 @@ type AdminUserListOptions struct {
ListOptions

// A search query string. Users are searchable by username and email address.
Query *string `url:"q,omitempty"`
Query string `url:"q,omitempty"`

// Can be "true" or "false" to show only administrators or non-administrators.
Administrators *string `url:"filter[admin]"`
Administrators string `url:"filter[admin]"`

// Can be "true" or "false" to show only suspended users or users who are not suspended.
SuspendedUsers *string `url:"filter[suspended]"`
SuspendedUsers string `url:"filter[suspended]"`

Include *[]AdminUserIncludeOps `url:"include,omitempty"`
Include []AdminUserIncludeOps `url:"include,omitempty"`
}

// List all user accounts in the Terraform Enterprise installation
Expand Down
12 changes: 6 additions & 6 deletions admin_user_integration_test.go
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@ func TestAdminUsers_List(t *testing.T) {

t.Run("query by username or email", func(t *testing.T) {
ul, err := client.Admin.Users.List(ctx, &AdminUserListOptions{
Query: String(currentUser.Username),
Query: currentUser.Username,
})
require.NoError(t, err)
assert.Equal(t, currentUser.ID, ul.Items[0].ID)
Expand All @@ -66,7 +66,7 @@ func TestAdminUsers_List(t *testing.T) {
defer memberCleanup()

ul, err = client.Admin.Users.List(ctx, &AdminUserListOptions{
Query: String(member.User.Email),
Query: member.User.Email,
})
require.NoError(t, err)
assert.Equal(t, member.User.Email, ul.Items[0].Email)
Expand All @@ -76,7 +76,7 @@ func TestAdminUsers_List(t *testing.T) {

t.Run("with organization included", func(t *testing.T) {
ul, err := client.Admin.Users.List(ctx, &AdminUserListOptions{
Include: &([]AdminUserIncludeOps{AdminUserOrgs}),
Include: []AdminUserIncludeOps{AdminUserOrgs},
})

assert.NoError(t, err)
Expand All @@ -88,7 +88,7 @@ func TestAdminUsers_List(t *testing.T) {

t.Run("filter by admin", func(t *testing.T) {
ul, err := client.Admin.Users.List(ctx, &AdminUserListOptions{
Administrators: String("true"),
Administrators: "true",
})

assert.NoError(t, err)
Expand Down Expand Up @@ -116,7 +116,7 @@ func TestAdminUsers_Delete(t *testing.T) {
member, _ := createOrganizationMembership(t, client, org)

ul, err := client.Admin.Users.List(ctx, &AdminUserListOptions{
Query: String(member.User.Email),
Query: member.User.Email,
})
require.NoError(t, err)
assert.Equal(t, member.User.Email, ul.Items[0].Email)
Expand All @@ -127,7 +127,7 @@ func TestAdminUsers_Delete(t *testing.T) {
require.NoError(t, err)

ul, err = client.Admin.Users.List(ctx, &AdminUserListOptions{
Query: String(member.User.Email),
Query: member.User.Email,
})
require.NoError(t, err)
assert.Empty(t, ul.Items)
Expand Down
4 changes: 2 additions & 2 deletions admin_workspace.go
View file
Open in desktop
Expand Up @@ -61,9 +61,9 @@ type AdminWorkspaceListOptions struct {

// A query string (partial workspace name) used to filter the results.
// https://www.terraform.io/docs/cloud/api/admin/workspaces.html#query-parameters
Query *string `url:"q,omitempty"`
Query string `url:"q,omitempty"`

Include *[]AdminWorkspaceIncludeOps `url:"include,omitempty"`
Include []AdminWorkspaceIncludeOps `url:"include,omitempty"`
}

// AdminWorkspaceList represents a list of workspaces.
Expand Down
8 changes: 4 additions & 4 deletions admin_workspace_integration_test.go
View file
Open in desktop
Expand Up @@ -65,7 +65,7 @@ func TestAdminWorkspaces_List(t *testing.T) {
// Use a known workspace prefix as search attribute. The result
// should be successful and only contain the matching workspace.
wl, err := client.Admin.Workspaces.List(ctx, &AdminWorkspaceListOptions{
Query: String(wTest1.Name),
Query: wTest1.Name,
})
require.NoError(t, err)
assert.Equal(t, adminWorkspaceItemsContainsID(wl.Items, wTest1.ID), true)
Expand All @@ -78,7 +78,7 @@ func TestAdminWorkspaces_List(t *testing.T) {
// Use a nonexisting workspace name as search attribute. The result
// should be successful, but return no results.
wl, err := client.Admin.Workspaces.List(ctx, &AdminWorkspaceListOptions{
Query: String("nonexisting"),
Query: "nonexisting",
})
require.NoError(t, err)
assert.Empty(t, wl.Items)
Expand All @@ -88,7 +88,7 @@ func TestAdminWorkspaces_List(t *testing.T) {

t.Run("with organization included", func(t *testing.T) {
wl, err := client.Admin.Workspaces.List(ctx, &AdminWorkspaceListOptions{
Include: &([]AdminWorkspaceIncludeOps{AdminWorkspaceOrg}),
Include: []AdminWorkspaceIncludeOps{AdminWorkspaceOrg},
})

assert.NoError(t, err)
Expand All @@ -109,7 +109,7 @@ func TestAdminWorkspaces_List(t *testing.T) {
assert.NoError(t, err)

wl, err := client.Admin.Workspaces.List(ctx, &AdminWorkspaceListOptions{
Include: &([]AdminWorkspaceIncludeOps{AdminWorkspaceCurrentRun}),
Include: []AdminWorkspaceIncludeOps{AdminWorkspaceCurrentRun},
})

assert.NoError(t, err)
Expand Down
2 changes: 1 addition & 1 deletion configuration_version.go
View file
Open in desktop
Expand Up @@ -117,7 +117,7 @@ type ConfigurationVersionReadOptions struct {
type ConfigurationVersionListOptions struct {
ListOptions

Include *[]ConfigurationVersionIncludeOps `url:"include,omitempty"`
Include []ConfigurationVersionIncludeOps `url:"include,omitempty"`
}

// IngressAttributes include commit information associated with configuration versions sourced from VCS.
Expand Down
2 changes: 1 addition & 1 deletion organization_tags.go
View file
Open in desktop
Expand Up @@ -46,7 +46,7 @@ type OrganizationTag struct {
type OrganizationTagsListOptions struct {
ListOptions

Filter *string `url:"filter[exclude][taggable][id],omitempty"`
Filter string `url:"filter[exclude][taggable][id],omitempty"`
}

// List all the tags in an organization. You can provide query params through OrganizationTagsListOptions
Expand Down
2 changes: 1 addition & 1 deletion organization_tags_integration_test.go
View file
Open in desktop
Expand Up @@ -63,7 +63,7 @@ func TestOrganizationTagsList(t *testing.T) {
PageNumber: 1,
PageSize: 5,
},
Filter: &testTagID,
Filter: testTagID,
})
require.NoError(t, err)

Expand Down
2 changes: 1 addition & 1 deletion policy.go
View file
Open in desktop
Expand Up @@ -84,7 +84,7 @@ type PolicyListOptions struct {
ListOptions

// A search string (partial policy name) used to filter the results.
Search *string `url:"search[name],omitempty"`
Search string `url:"search[name],omitempty"`
}

// List all the policies for a given organization
Expand Down
2 changes: 1 addition & 1 deletion policy_integration_test.go
View file
Open in desktop
Expand Up @@ -60,7 +60,7 @@ func TestPoliciesList(t *testing.T) {
// Search by one of the policy's names; we should get only that policy
// and pagination data should reflect the search as well
pl, err := client.Policies.List(ctx, orgTest.Name, &PolicyListOptions{
Search: &pTest1.Name,
Search: pTest1.Name,
})
require.NoError(t, err)

Expand Down
2 changes: 1 addition & 1 deletion policy_set.go
View file
Open in desktop
Expand Up @@ -93,7 +93,7 @@ type PolicySetListOptions struct {
ListOptions

// A search string (partial policy set name) used to filter the results.
Search *string `url:"search[name],omitempty"`
Search string `url:"search[name],omitempty"`
}

// List all the policies for a given organization.
Expand Down
2 changes: 1 addition & 1 deletion policy_set_integration_test.go
View file
Open in desktop
Expand Up @@ -58,7 +58,7 @@ func TestPolicySetsList(t *testing.T) {
// Search by one of the policy set's names; we should get only that policy
// set and pagination data should reflect the search as well
psl, err := client.PolicySets.List(ctx, orgTest.Name, &PolicySetListOptions{
Search: String(psTest1.Name),
Search: psTest1.Name,
})
require.NoError(t, err)

Expand Down
2 changes: 1 addition & 1 deletion run.go
View file
Open in desktop
Expand Up @@ -174,7 +174,7 @@ const (
type RunListOptions struct {
ListOptions

Include *[]RunIncludeOps `url:"include,omitempty"`
Include []RunIncludeOps `url:"include,omitempty"`
}

// RunVariable represents a variable that can be applied to a run. All values must be expressed as an HCL literal
Expand Down