Skip to content

chore: migrate tfe_oauth_client to framework #1661

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
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

chore: migrate tfe_oauth_client to framework #1661

wants to merge 5 commits into from

Conversation

ctrombley
Copy link
Contributor

@ctrombley ctrombley commented Mar 22, 2025 •
edited
Loading

Description

This PR migrates the tfe_oauth_client resource and data source to the provider framework.

Acceptance tests that made use of any of the above also needed to be switched to the muxed provider factory.

Testing plan

  1. acc tests
  2. all data sources and resources should continue to function as usual

Output from acceptance tests

$ TESTARGS="-run TestAccTFEOAuthClient_" make testacc

=== RUN   TestAccTFEOAuthClient_basic
--- PASS: TestAccTFEOAuthClient_basic (3.99s)
=== RUN   TestAccTFEOAuthClient_rsaKeys
--- PASS: TestAccTFEOAuthClient_rsaKeys (3.51s)
=== RUN   TestAccTFEOAuthClient_agentPool
    helper_test.go:237: Skipping test related to a HCP Terraform and Terraform Enterprise beta feature. Set ENABLE_BETA=1 to run.
--- SKIP: TestAccTFEOAuthClient_agentPool (0.00s)
PASS
ok      github.com/hashicorp/terraform-provider-tfe/internal/provider   7.995s

$ TESTARGS="-run TestAccTFEOAuthClientDataSource_" make testacc
=== RUN   TestAccTFEOAuthClientDataSource_basic
--- PASS: TestAccTFEOAuthClientDataSource_basic (7.38s)
=== RUN   TestAccTFEOAuthClientDataSource_findByID
--- PASS: TestAccTFEOAuthClientDataSource_findByID (4.37s)
=== RUN   TestAccTFEOAuthClientDataSource_findByName
--- PASS: TestAccTFEOAuthClientDataSource_findByName (4.40s)
=== RUN   TestAccTFEOAuthClientDataSource_findByServiceProvider
--- PASS: TestAccTFEOAuthClientDataSource_findByServiceProvider (4.78s)
=== RUN   TestAccTFEOAuthClientDataSource_missingParameters
--- PASS: TestAccTFEOAuthClientDataSource_missingParameters (0.46s)
=== RUN   TestAccTFEOAuthClientDataSource_sameName
--- PASS: TestAccTFEOAuthClientDataSource_sameName (3.08s)
=== RUN   TestAccTFEOAuthClientDataSource_noName
--- PASS: TestAccTFEOAuthClientDataSource_noName (2.78s)
=== RUN   TestAccTFEOAuthClientDataSource_sameServiceProvider
--- PASS: TestAccTFEOAuthClientDataSource_sameServiceProvider (3.31s)
PASS
ok      github.com/hashicorp/terraform-provider-tfe/internal/provider   31.363s

@ctrombley ctrombley self-assigned this Mar 22, 2025
@ctrombley ctrombley requested a review from a team as a code owner March 22, 2025 01:10
@ctrombley ctrombley force-pushed the TF-25009-add-write-only-attributes-for-tfe-oauth-client branch 2 times, most recently from df31b92 to 1a58dc0 Compare March 27, 2025 03:54
@ctrombley ctrombley mentioned this pull request Mar 27, 2025
Merged
2 tasks
@ctrombley ctrombley force-pushed the TF-25009-add-write-only-attributes-for-tfe-oauth-client branch from 1a58dc0 to ba098cb Compare March 28, 2025 01:33
uturunku1
uturunku1 suggested changes Mar 28, 2025
View reviewed changes
internal/provider/resource_tfe_oauth_client.go
Optional: true,
Default: true,

"organization_scoped": schema.BoolAttribute{
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these last three attributes should require replacement. For example when testing with the previous sdk, switching the value from organization_scoped from true to false causes an update-in-place. Whereas with this change, it forces a replacement and it causes the error Provider produced inconsistent result after apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks! Updated in a6d5a36.

@ctrombley ctrombley force-pushed the TF-25009-add-write-only-attributes-for-tfe-oauth-client branch from e6ef965 to a6d5a36 Compare March 28, 2025 22:16
@ctrombley ctrombley requested a review from uturunku1 March 28, 2025 22:17
ctrombley
ctrombley commented Mar 29, 2025
View reviewed changes
internal/provider/data_source_oauth_client_test.go
Comment on lines -146 to +140
func TestAccTFEOAuthClientDataSource_missingOrgWithName(t *testing.T) {
rInt := rand.New(rand.NewSource(time.Now().UnixNano())).Int()
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccTFEOAuthClientDataSourcePreCheck(t) },
ProtoV5ProviderFactories: testAccMuxedProviders,
Steps: []resource.TestStep{
{
Config: testAccTFEOAuthClientDataSourceConfig_missingOrgWithName(rInt),
ExpectError: regexp.MustCompile("all of `name,organization` must"),
},
},
})
}

func TestAccTFEOAuthClientDataSource_missingOrgWithServiceProvider(t *testing.T) {
rInt := rand.New(rand.NewSource(time.Now().UnixNano())).Int()
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccTFEOAuthClientDataSourcePreCheck(t) },
ProtoV5ProviderFactories: testAccMuxedProviders,
Steps: []resource.TestStep{
{
Config: testAccTFEOAuthClientDataSourceConfig_missingOrgWithServiceProvider(rInt),
ExpectError: regexp.MustCompile("all of `organization,service_provider` must be"),
ExpectError: regexp.MustCompile("Invalid Attribute Combination"),
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 removed these tests since we are now defaulting organization to the provider config.

@ctrombley ctrombley force-pushed the TF-25009-add-write-only-attributes-for-tfe-oauth-client branch 2 times, most recently from 7b65e2a to 9352b04 Compare March 31, 2025 18:13
uturunku1
uturunku1 suggested changes Mar 31, 2025
View reviewed changes
internal/provider/data_source_oauth_client.go Outdated
oc, err = d.config.Client.OAuthClients.Read(ctx, id)
if err != nil && errors.Is(err, tfe.ErrResourceNotFound) {
tflog.Debug(ctx, fmt.Sprintf("OAuth client %s no longer exists", id))
resp.State.RemoveResource(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

This removeResource check is only applicable to resource. If you run into the "resource not found" error when accessing a data source, it means that you passed the wrong id and no resource is associated with that client id. So I think here we just want to do:

oc, err = d.config.Client.OAuthClients.Read(ctx, id)
if err != nil {
			resp.Diagnostics.AddError("Error reading OAuth client", err.Error())
			return
		}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in b2339ec.

@uturunku1
Copy link
Contributor

I also noticed that there was pre-existing issue with the oauth client resource, where setting these two attributes organization and organization_scoped = false causes this error:

Screenshot 2025-03-31 at 12 20 36 PM

And then if you attempt to remove organization_scoped = false from the configuration, you can't because the error continues to show up. Maybe the easiest solution on our end would be to set a conflictsWith between those two attributes?

@ctrombley ctrombley force-pushed the TF-25009-add-write-only-attributes-for-tfe-oauth-client branch from 9352b04 to b2339ec Compare March 31, 2025 19:32
@ctrombley ctrombley requested a review from uturunku1 March 31, 2025 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uturunku1 uturunku1 Awaiting requested review from uturunku1

At least 1 approving review is required to merge this pull request.

Assignees

@ctrombley ctrombley

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ctrombley @uturunku1