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.

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

add missing include field for listOptions #339

Merged
merged 1 commit into from Mar 1, 2022

Conversation

uturunku1
Copy link
Contributor

@uturunku1 uturunku1 commented Mar 1, 2022

Description

Add "include" field to listOptions structs for API that allow for include values.

These are the APIs and their include values listed in Atlas:

agent pool API: "workspaces"
oauth client API: "oauth_tokens"
policy check API: "run.workspace", "run"
run trigger API: "workspace", "sourceable"
organization API: "entitlement_set"
policy set versions API: "policy_set"
policy API: "policy_sets"

I did not implemented the changes for:

organizations API - because the relation for entitlement set was not part of type Organization struct.
policy set version API - I did have the relation present but I did not have the listOptions or listing function present.
Policy API - Policies controller has the include value "policy_sets", but this value does not align with only relation listed under type Policy struct: Organization *Organization jsonapi:"relation,organization"`` What am I missing here?

All the other include fields were added. This is how the implementation was made:

Step 1 - Add include struct field to SomeResourceListOptions
Step 2 - Create constants for the allowed include values that are typed strings
Step 3 - Add a test to make sure what are getting the "included" data

Bonus Step:

Change the pre-existing typed string named SomePrefixIncludeOps to SomePrefixIncludeOp, like type OAuthClientIncludeOps string becomes type OAuthClientIncludeOp string. I should have made the suffix singular from the beginning, but it is not too late to make that correction.
To reduce noise coming from this change, you can focus on the changes made to:

  1. agent_pool.go
  2. agent_pool_integration_test.go
  3. oauth_client.go
  4. oauth_client_integration_test.go
  5. policy_check.go
  6. policy_check_integration_test.go
  7. run_trigger.go
  8. run_trigger_integration_test.go
  9. errors.go

Testing plan

External links

Output from tests (HashiCorp employees only)

$ TFE_ADDRESS="https://example" TFE_TOKEN="example" TF_ACC="1" go test ./... -v -tags=integration -run TestFunctionsAffectedByChange

...

oauth_client.go Outdated
// A list of relations to include
type OAuthClientIncludeOp string

const OauthClientOauthTokens OAuthClientIncludeOp = "oauth_tokens"
Copy link
Contributor Author

@uturunku1 uturunku1 Mar 1, 2022

Choose a reason for hiding this comment

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

Besides "OAuthToken" another relation listed for type OAuthClient struct is "organization". However, I did not included a constant for "organization" because I did not see it listed in atlas's allowed_values_for_include

@uturunku1 uturunku1 marked this pull request as ready for review Mar 1, 2022
@uturunku1
Copy link
Contributor Author

uturunku1 commented Mar 1, 2022

I will squash these commits after PR approval

@sebasslash
Copy link
Contributor

sebasslash commented Mar 1, 2022

I have two comments:

  1. I have a hard time distinguishing what IncludeOp means... Include Operation? Include Option? I've typically seen Op for operation and Opt for option.
  2. A lot of these strings are not documented and thus will not show up on GoDocs (or am I wrong?)... so while we've prevented a customer from sending an invalid include value, the customer would still need to reference the API docs AND the code to see what include options are available. Should we make this easier for them?

@uturunku1
Copy link
Contributor Author

uturunku1 commented Mar 1, 2022

I have two comments:

  1. I have a hard time distinguishing what IncludeOp means... Include Operation? Include Option? I've typically seen Op for operation and Opt for option.
  2. A lot of these strings are not documented and thus will not show up on GoDocs (or am I wrong?)... so while we've prevented a customer from sending an invalid include value, the customer would still need to reference the API docs AND the code to see what include options are available. Should we make this easier for them?
  1. You are right that many people may interpret Op for operations. I'll switch it to IncludeOpt
  2. They are not documented because unfortunately API docs does not have any documentation on it :(

Copy link
Contributor

@sebasslash sebasslash left a comment

Last teeny bit, I'm sorry 😢

run_task.go Outdated
}

// RunTaskReadOptions represents the set of options for reading a run task
type RunTaskReadOptions struct {
// Optional: A list of relations to include with a run task. See available resources:
// https://www.terraform.io/cloud-docs/api-docs/run-tasks#list-run-tasks
Include []RunTaskIncludeOps `url:"include"`
Include []RunTaskIncludeOpt `url:"include"`
Copy link
Contributor

@sebasslash sebasslash Mar 1, 2022

Choose a reason for hiding this comment

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

Ooo a missing omitempty ? 🤔

Copy link
Contributor Author

@uturunku1 uturunku1 Mar 1, 2022

Choose a reason for hiding this comment

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

lol, I don't apply the practices I preach. Making that change now!

@uturunku1 uturunku1 merged commit f6d1599 into releases-1.0.x Mar 1, 2022
4 checks passed
@uturunku1 uturunku1 deleted the update-include-field branch Mar 1, 2022
uturunku1 added a commit that referenced this issue Mar 3, 2022
@uturunku1 uturunku1 mentioned this pull request Mar 3, 2022
uturunku1 added a commit that referenced this issue Mar 3, 2022
uturunku1 added a commit that referenced this issue Mar 4, 2022
uturunku1 added a commit that referenced this issue Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants