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

create oauth_client with agent pool #1255

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

roleesinhaHC
Copy link
Contributor

@roleesinhaHC roleesinhaHC commented Feb 14, 2024

Description

Adds Private VCS support by creating OAuth Client with Agent Pool.

Remember to:

Testing plan

  1. Create a test org with private VCS enabled.
  2. Create an agent pool, and launch an local agent supporting request forwarding.
  3. Create OAuth Client with agent pool.

Output from acceptance tests

Please run applicable acceptance tests locally and include the output here. See testing.md to learn how to run acceptance tests.

If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.

terraform-provider-tfe % TFE_TOKEN=xxx TFE_HOSTNAME=app.terraform.io GITHUB_TOKEN=ghp_xxx TESTARGS="-run TestAccTFEOAuthClient_agentPool" make testacc
TF_ACC=1 TF_LOG_SDK_PROTO=OFF go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEOAuthClient_agentPool -timeout 15m
?       github.com/hashicorp/terraform-provider-tfe     [no test files]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-tfe/internal/client     (cached) [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-tfe/internal/logging    (cached) [no tests to run]
?       github.com/hashicorp/terraform-provider-tfe/version     [no test files]
=== RUN   TestAccTFEOAuthClient_agentPool
--- PASS: TestAccTFEOAuthClient_agentPool (8.89s)
PASS
ok      github.com/hashicorp/terraform-provider-tfe/internal/provider   9.358s
...

Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

Gave it a first-pass. Great work Rolee!

go.sum Outdated
Comment on lines 74 to 75
github.com/hashicorp/go-tfe v1.45.0 h1:WCiQWUV7n1Fq/pKA9C3rhcSmUtSPTYBtE1kIJ9U0NSU=
github.com/hashicorp/go-tfe v1.45.0/go.mod h1:GRvhVp0mlNK/msPAvdeubWnV57avNoCmeaetcmvUyHY=
Copy link
Contributor

Choose a reason for hiding this comment

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

you'll want to run go mod tidy to remove the old version references.

Comment on lines 115 to 118
"agent_pool_id": {
Type: schema.TypeString,
ForceNew: true,
},
Copy link
Contributor

@sebasslash sebasslash Mar 1, 2024

Choose a reason for hiding this comment

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

We're missing a few fields here, but I'm not fully familiar with the internals of the OAuth client API:
If the user must specify this attribute in their configuration, we'll need Require: true, otherwise Optional: true. If this agent pool ID can be a value read from the API, we should also specify Computed: true -- otherwise if agent_pool_id is changed in the API, the provider will re-create the resource and set the agent_pool_id to whatever was originally in your statefile.

Normally you'll use these schema behaviors like so:

// Indicates an attribute *must* be strictly managed by Terraform. If the attribute in the API changes, Terraform
// will remove it and recreate it to match your config. 
"some_attribute": {
  Type: schema.SomeType,
  Required: true,
  ForceNew: true
}

or

// Indicates an attribute that can either be managed by Terraform or a value read from the API. 
"some_attribute": {
  Type: schema.SomeType,
  Optional: true,
  Computed: true
}

I would avoid mixing Optional/Computed and ForceNew for any unintended side effects. But someone feel free to correct me if I'm wrong here 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @sebasslash for the detailed explanation. I'll make the code changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebasslash should I keep it on draft state since its in beta, or merge it and update the change log later? What do you recommend. thanks!

Copy link
Collaborator

@uturunku1 uturunku1 left a comment

Choose a reason for hiding this comment

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

Great 👍 Thanks for updating the test and the suggested changes. I have approve for now, but if you make additional changes closer to the release date of the feature, please request a new approval from the team.

@roleesinhaHC roleesinhaHC marked this pull request as ready for review March 11, 2024 21:27
@roleesinhaHC roleesinhaHC requested a review from a team as a code owner March 11, 2024 21:27
Copy link
Contributor

@Maed223 Maed223 left a comment

Choose a reason for hiding this comment

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

Tiniest thing, but your @ in the Changelog should be your GH username "@roleesinhaHC" rather than "rolee.sinha"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants