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

resource/github_repository_deploy_key: Trim key #132

Merged
merged 2 commits into from
Aug 14, 2018
Merged

resource/github_repository_deploy_key: Trim key #132

merged 2 commits into from
Aug 14, 2018

Conversation

radeksimko
Copy link
Contributor

Fixes #69

TF_ACC=1 go test ./github -v -run=TestAccGithubRepositoryDeployKey_ -timeout 120m
=== RUN   TestAccGithubRepositoryDeployKey_basic
--- PASS: TestAccGithubRepositoryDeployKey_basic (4.29s)
=== RUN   TestAccGithubRepositoryDeployKey_importBasic
--- PASS: TestAccGithubRepositoryDeployKey_importBasic (3.69s)
PASS
ok  	github.com/terraform-providers/terraform-provider-github/github	8.108s

@radeksimko radeksimko added the Type: Bug Something isn't working as documented label Aug 13, 2018
@radeksimko radeksimko force-pushed the b-trim-repo-deploy-key branch from 85d0086 to 9c01db3 Compare August 13, 2018 11:34
appilon
appilon suggested changes Aug 13, 2018
View reviewed changes
github/resource_github_repository_deploy_key_test.go Outdated
oldV := "ssh-rsa AAAABB...cd+== terraform-acctest@hashicorp.com\n"
newV := "ssh-rsa AAAABB...cd+=="
if suppressDeployKeyDiff("test", oldV, newV, nil) {
t.Fatalf("Expected %q and %q to be suppressed", oldV, newV)
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean NOT suppressed in the error message?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only real blocker for me on the PR, either the message is missing NOT or the conditional should be negated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, the reason the test was passing is because I accidentally swapped oldV and newV. 🙈
Fixed now!

github/resource_github_repository_deploy_key_test.go
@@ -11,21 +13,43 @@ import (
"github.com/hashicorp/terraform/terraform"
)

func TestSuppressDeployKeyDiff(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really strongly opinionated on this, but typically I do 1 validation per test and typically the function name is quite verbose, almost describing the test circumstance. What is more important though is our unit test style is consistent, so if this is consistent with everything it's all good with me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair point, I'll break it apart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually we do have many unit tests (set aside acceptance tests which can have multiple steps) which have multiple test cases in them and I think it's fine? e.g. https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/validators_test.go#L12-L54

The thing which I agree needs changing though is the error message, so it's more obvious where each failure is coming from. 👍 I will fix that.

@radeksimko radeksimko force-pushed the b-trim-repo-deploy-key branch from 9c01db3 to c50e02a Compare August 14, 2018 10:36
@radeksimko radeksimko force-pushed the b-trim-repo-deploy-key branch from 28d1145 to b417dbd Compare August 14, 2018 10:48
appilon
appilon approved these changes Aug 14, 2018
View reviewed changes
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

github/resource_github_repository_deploy_key_test.go
@@ -11,21 +13,60 @@ import (
"github.com/hashicorp/terraform/terraform"
)

func TestSuppressDeployKeyDiff(t *testing.T) {
testCases := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice table driven tests!

@radeksimko radeksimko merged commit 2ddac69 into master Aug 14, 2018
@radeksimko radeksimko deleted the b-trim-repo-deploy-key branch August 14, 2018 13:42
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
@radeksimko
Merge pull request integrations#132 from terraform-providers/b-trim-r…
51ff8f1 
…epo-deploy-key

resource/github_repository_deploy_key: Trim key
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@appilon appilon appilon approved these changes

Assignees
No one assigned
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Milestone
No milestone
Development

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