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.

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

New Resource: github_user_gpg_key #120

Merged
merged 1 commit into from
Aug 10, 2018
Merged

New Resource: github_user_gpg_key #120

merged 1 commit into from
Aug 10, 2018

Conversation

radeksimko
Copy link
Contributor 

TF_ACC=1 go test ./github -v -run=TestAccGithubUserGpgKey_ -timeout 120m
=== RUN   TestAccGithubUserGpgKey_basic
--- PASS: TestAccGithubUserGpgKey_basic (2.17s)
PASS
ok  	github.com/terraform-providers/terraform-provider-github/github	2.281s

catsby
catsby approved these changes Aug 9, 2018
View reviewed changes
Copy link

@catsby catsby left a comment

Choose a reason for hiding this comment

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

One nit about logging when removing from state. Other comments can safely be ignored 😄

github/resource_github_user_gpg_key.go Outdated
key, _, err := client.Users.GetGPGKey(context.TODO(), id)
if err != nil {
d.SetId("")
return nil
Copy link

Choose a reason for hiding this comment

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

Should log that the d.Id() wasn't found and removed from state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

github/resource_github_user_gpg_key.go
return err
}

d.SetId(strconv.FormatInt(*key.ID, 10))
Copy link

Choose a reason for hiding this comment

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

Should we use key.KeyID here instead? Looking at GitHub docs it looks like key.ID is a database index-like sequential key, where as KeyID appears to be a unique id.

It also saves us the trouble of using FormatInt and error checking

Copy link

Choose a reason for hiding this comment

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

I guess that depends on what GetGPGKey uses... the docs make it look like it uses key_id but I can't imagine your tests passed if that's what they expected and you weren't doing that 🤔

Copy link

Choose a reason for hiding this comment

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

I've answered my own question. How bizarre 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The documentation is indeed a bit confusing on this one. I actually first tried to use the KeyID, before realizing it's the numeric ID and that the interface expects *int. 😄

@radeksimko radeksimko merged commit c6a6e49 into master Aug 10, 2018
@radeksimko radeksimko deleted the f-user-gpg-key branch August 10, 2018 05:40
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
@radeksimko
Merge pull request integrations#120 from terraform-providers/f-user-g…
0b73103 
…pg-key

New Resource: github_user_gpg_key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@catsby catsby catsby approved these changes

Assignees
No one assigned
Labels
New resource
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@radeksimko @catsby