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

New datasource: tfe_oauth_client #212

Merged
merged 11 commits into from
Sep 23, 2020
Merged

New datasource: tfe_oauth_client #212

merged 11 commits into from
Sep 23, 2020

Conversation

koikonom
Copy link
Contributor

@koikonom koikonom commented Sep 3, 2020

Description

This is a new datasource that allows users to extract information from
existing oauth clients.

Output from acceptance tests

❯ TESTARGS='-run TestAccTFEOAuthClientDataSource_basic' make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccTFEOAuthClientDataSource_basic -timeout 15m
?       github.com/terraform-providers/terraform-provider-tfe   [no test files]
=== RUN   TestAccTFEOAuthClientDataSource_basic
--- PASS: TestAccTFEOAuthClientDataSource_basic (29.90s)
PASS
ok      github.com/terraform-providers/terraform-provider-tfe/tfe       30.846s
?       github.com/terraform-providers/terraform-provider-tfe/version   [no test files]

This is a new datasource that allows users to extract information from
existing oauth clients.
@ghost ghost added the size/M label Sep 3, 2020
@ghost ghost added the documentation label Sep 3, 2020
@koikonom
Copy link
Contributor Author

koikonom commented Sep 3, 2020

Closes #10

Copy link
Contributor

@lafentres lafentres left a comment

Choose a reason for hiding this comment

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

i'm working on getting a local build of this set up but wanted to get you my code review part sooner rather than later :]
everything looks good! just some minor comments on style/naming and some typo fixes

return fmt.Errorf("Error retrieving OAuth client: %v", err)
}

tokenID := oc.OAuthTokens[0].ID
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think it is worth handling this case the way we do in the resource? in case the oauth client doesn't have an oauth token yet?

like this: https://github.com/terraform-providers/terraform-provider-tfe/blob/master/tfe/resource_tfe_oauth_client.go#L130

Copy link
Contributor

Choose a reason for hiding this comment

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

can confirm that we need to handle this similar to how we handle it in the resource. this panics if you give it the oauth client id of an unfinished client (so a client that doesn't have an oauth token yet)

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 for the hint!

tfe/data_source_oauth_client.go Outdated Show resolved Hide resolved
tfe/data_source_oauth_client.go Outdated Show resolved Hide resolved
website/docs/d/oauth_client.html.markdown Outdated Show resolved Hide resolved
website/tfe.erb Outdated Show resolved Hide resolved
tfe/provider.go Outdated Show resolved Hide resolved
tfe/data_source_oauth_client_test.go Outdated Show resolved Hide resolved
}

func dataSourceTFEOAuthClientRead(d *schema.ResourceData, meta interface{}) error {
ctx := context.TODO()
Copy link
Contributor

Choose a reason for hiding this comment

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

just a question for my own learning: i've never seen this before. i read about it in the docs and it makes sense. is this something we should be doing in all of our resources/data sources that have a blank/empty context?

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 created an empty context because I needed one to pass to the tfe client.

Perhaps in the long run we could (should?) revisit the codebase and set a context with a timeout (https://godoc.org/context#WithTimeout).

koikonom and others added 7 commits September 9, 2020 19:12
Co-authored-by: Krista LaFentres <lafentres@users.noreply.github.com>
Co-authored-by: Krista LaFentres <lafentres@users.noreply.github.com>
Co-authored-by: Krista LaFentres <lafentres@users.noreply.github.com>
Co-authored-by: Krista LaFentres <lafentres@users.noreply.github.com>
@ghost ghost added size/L and removed size/M labels Sep 9, 2020
Comment on lines 31 to 34
"ssh_key": {
Type: schema.TypeString,
Computed: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

i totally missed this when i was looking at this before but this is always going to be blank unless the oauth client is bitbucket server. it would be fine to leave it in but i think we should rename it to match go-tfe and the api response which is rsa public key.
if you end up leaving this in, it would be good to note that this is only available on bitbucket server oauth clients.

Suggested change
"ssh_key": {
Type: schema.TypeString,
Computed: true,
},
"rsa_public_key": {
Type: schema.TypeString,
Computed: true,
},

Copy link
Contributor

Choose a reason for hiding this comment

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

i've confirmed that the rsa_public_key is only every populated for bitbucket server connections and isn't something that would be useful so it can just be removed.

website/tfe.erb Outdated
@@ -13,6 +13,9 @@
<li<%= sidebar_current("docs-tfe-datasource") %>>
<a href="#">Data Sources</a>
<ul class="nav nav-visible">
<li<%= sidebar_current("docs-datasource-tfe-oauth-client") %>>
Copy link
Contributor

@lafentres lafentres Sep 10, 2020

Choose a reason for hiding this comment

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

i think this might need the -x at the end to match the sidebar_current value in the website/docs/d/oauth_client.html.markdown file. i think this might not work as expected otherwise.
you could also just remove the -x from the sidebar_current value in the website/docs/d/oauth_client.html.markdown file i think.

Copy link
Contributor

@lafentres lafentres left a comment

Choose a reason for hiding this comment

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

this looks good to me! once the ssh_key/rsa_public_key is removed and the docs sidebar typo is fixed this should be good to go.

SSH key is only used for Bitbucket VCS connection and we don't really
need it exposed.

... also minor doc fix.
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