-
Notifications
You must be signed in to change notification settings - Fork 160
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
Conversation
This is a new datasource that allows users to extract information from existing oauth clients.
Closes #10 |
There was a problem hiding this 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
tfe/data_source_oauth_client.go
Outdated
return fmt.Errorf("Error retrieving OAuth client: %v", err) | ||
} | ||
|
||
tokenID := oc.OAuthTokens[0].ID |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hint!
} | ||
|
||
func dataSourceTFEOAuthClientRead(d *schema.ResourceData, meta interface{}) error { | ||
ctx := context.TODO() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
Co-authored-by: Krista LaFentres <[email protected]>
Co-authored-by: Krista LaFentres <[email protected]>
Co-authored-by: Krista LaFentres <[email protected]>
Co-authored-by: Krista LaFentres <[email protected]>
tfe/data_source_oauth_client.go
Outdated
"ssh_key": { | ||
Type: schema.TypeString, | ||
Computed: true, | ||
}, |
There was a problem hiding this comment.
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.
"ssh_key": { | |
Type: schema.TypeString, | |
Computed: true, | |
}, | |
"rsa_public_key": { | |
Type: schema.TypeString, | |
Computed: true, | |
}, |
There was a problem hiding this comment.
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") %>> |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
Description
This is a new datasource that allows users to extract information from
existing oauth clients.
Output from acceptance tests