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

Add repository topics #97

Merged
merged 3 commits into from
Jun 1, 2018

Conversation

endorama
Copy link
Contributor

Hello, first contribution here! First of all thanks for the project.

Following #41, this PR should add topics support to github_repository terraform resource.

I'm having some trouble compiling due to an error: ( on master branch )

./main.go:10:15: cannot use "github.com/terraform-providers/terraform-provider-github/github".Provider (type func() "github.com/terraform-providers/terraform-provider-github/vendor/github.com/hashicorp/terraform/terraform".ResourceProvider) as type "github.com/endorama/terraform-provider-github/vendor/github.com/hashicorp/terraform/plugin".ProviderFunc in field value

so I can't run the acceptance tests ( which are included, but untested ).

It's also my first terraform provider contribution and I'm not so skilled in go, so please advice on any error I missed 🙂

cc @paultyng ( due to his last comment in #41 )

Thank you!

Copy link
Contributor

@paultyng paultyng left a comment

Choose a reason for hiding this comment

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

I don't think the acceptance tests will pass as is due to a conversion error, see my comment inline.

@@ -137,6 +142,7 @@ func resourceGithubRepositoryObject(d *schema.ResourceData) *github.Repository {
licenseTemplate := d.Get("license_template").(string)
gitIgnoreTemplate := d.Get("gitignore_template").(string)
archived := d.Get("archived").(bool)
topics := d.Get("topics").([]string)
Copy link
Contributor

@paultyng paultyng May 29, 2018

Choose a reason for hiding this comment

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

This will come out of ResourceData as []interface{}, so you will need to write a simple func to "expand" this state to the SDK's representation:

func expandStringList(configured []interface{}) []string {
	vs := make([]string, 0, len(configured))
	for _, v := range configured {
		val, ok := v.(string)
		if ok && val != "" {
			vs = append(vs, val)
		}
	}
	return vs
}

@@ -377,6 +405,9 @@ func testAccCheckGithubRepositoryAttributes(repo *github.Repository, want *testA
if *repo.HasDownloads != want.HasDownloads {
return fmt.Errorf("got has downloads %#v; want %#v", *repo.HasDownloads, want.HasDownloads)
}
if &repo.Topics != &want.Topics {
Copy link
Contributor

Choose a reason for hiding this comment

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

You will probably need to rewrite this comparison somewhat, as the default value for a slice is nil but its equivalent to an empty slice, so you'll want to check len and iterate:

if len(want.Topics) != len(repo.Topics) {
			return fmt.Errorf("got has topics %#v; want %#v", repo.Topics, want.Topics)
		}
		for i := range want.Topics {
			if repo.Topics[i] != want.Topics[i] {
				return fmt.Errorf("got has topics %#v; want %#v", repo.Topics, want.Topics)
			}
		}

@paultyng paultyng dismissed their stale review May 30, 2018 00:19

code change

@paultyng
Copy link
Contributor

paultyng commented May 30, 2018

This one was a little weird as topics actually needed to be updated via a different API call, so I went ahead and pushed a new commit with the modifications, I'll need to get someone else to do a review.

Ideally you would run the updated acceptance testing prior to opening the PR, obviously there is sometimes cost associated so its understandable in situations where you can't, but in this case it would have probably helped you find some of these issues sooner.

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@paultyng
Copy link
Contributor

paultyng commented Jun 1, 2018

$ make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v  -timeout 120m
?   	github.com/terraform-providers/terraform-provider-github	[no test files]
=== RUN   TestAccGithubIpRangesDataSource_existing
--- PASS: TestAccGithubIpRangesDataSource_existing (0.69s)
=== RUN   TestAccGithubTeamDataSource_noMatchReturnsError
--- PASS: TestAccGithubTeamDataSource_noMatchReturnsError (0.07s)
=== RUN   TestAccGithubUserDataSource_noMatchReturnsError
--- PASS: TestAccGithubUserDataSource_noMatchReturnsError (0.07s)
=== RUN   TestAccGithubUserDataSource_existing
--- PASS: TestAccGithubUserDataSource_existing (0.94s)
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccGithubBranchProtection_basic
--- PASS: TestAccGithubBranchProtection_basic (4.42s)
=== RUN   TestAccGithubBranchProtection_emptyItems
--- PASS: TestAccGithubBranchProtection_emptyItems (3.52s)
=== RUN   TestAccGithubBranchProtection_importBasic
--- PASS: TestAccGithubBranchProtection_importBasic (3.12s)
=== RUN   TestAccGithubIssueLabel_basic
--- PASS: TestAccGithubIssueLabel_basic (3.29s)
=== RUN   TestAccGithubIssueLabel_existingLabel
--- PASS: TestAccGithubIssueLabel_existingLabel (2.25s)
=== RUN   TestAccGithubIssueLabel_importBasic
--- PASS: TestAccGithubIssueLabel_importBasic (2.07s)
=== RUN   TestAccGithubMembership_basic
--- PASS: TestAccGithubMembership_basic (1.10s)
=== RUN   TestAccGithubMembership_importBasic
--- PASS: TestAccGithubMembership_importBasic (0.94s)
=== RUN   TestAccGithubOrganizationWebhook_basic
--- PASS: TestAccGithubOrganizationWebhook_basic (0.94s)
=== RUN   TestAccGithubRepositoryCollaborator_basic
--- PASS: TestAccGithubRepositoryCollaborator_basic (3.18s)
=== RUN   TestAccGithubRepositoryCollaborator_importBasic
--- PASS: TestAccGithubRepositoryCollaborator_importBasic (2.72s)
=== RUN   TestAccGithubRepositoryDeployKey_basic
--- PASS: TestAccGithubRepositoryDeployKey_basic (2.36s)
=== RUN   TestAccGithubRepositoryDeployKey_importBasic
--- PASS: TestAccGithubRepositoryDeployKey_importBasic (2.59s)
=== RUN   TestAccGithubRepository_basic
--- PASS: TestAccGithubRepository_basic (2.86s)
=== RUN   TestAccGithubRepository_archive
--- PASS: TestAccGithubRepository_archive (2.50s)
=== RUN   TestAccGithubRepository_archiveUpdate
--- PASS: TestAccGithubRepository_archiveUpdate (2.38s)
=== RUN   TestAccGithubRepository_importBasic
--- PASS: TestAccGithubRepository_importBasic (2.04s)
=== RUN   TestAccGithubRepository_defaultBranch
--- PASS: TestAccGithubRepository_defaultBranch (4.92s)
=== RUN   TestAccGithubRepository_templates
--- PASS: TestAccGithubRepository_templates (3.01s)
=== RUN   TestAccGithubRepository_topics
--- PASS: TestAccGithubRepository_topics (5.44s)
=== RUN   TestAccGithubRepositoryWebhook_basic
--- PASS: TestAccGithubRepositoryWebhook_basic (3.23s)
=== RUN   TestAccGithubRepositoryWebhook_importBasic
--- PASS: TestAccGithubRepositoryWebhook_importBasic (2.47s)
=== RUN   TestAccGithubTeamMembership_basic
--- PASS: TestAccGithubTeamMembership_basic (2.43s)
=== RUN   TestAccGithubTeamMembership_importBasic
--- PASS: TestAccGithubTeamMembership_importBasic (2.25s)
=== RUN   TestAccGithubTeamRepository_basic
--- PASS: TestAccGithubTeamRepository_basic (4.44s)
=== RUN   TestAccGithubTeamRepository_importBasic
--- PASS: TestAccGithubTeamRepository_importBasic (2.31s)
=== RUN   TestAccCheckGetPermissions
--- PASS: TestAccCheckGetPermissions (0.00s)
=== RUN   TestAccGithubTeam_basic
--- PASS: TestAccGithubTeam_basic (1.22s)
=== RUN   TestAccGithubTeam_hierarchical
--- PASS: TestAccGithubTeam_hierarchical (1.46s)
=== RUN   TestAccGithubTeam_importBasic
--- PASS: TestAccGithubTeam_importBasic (0.83s)
=== RUN   TestAccGithubUtilRole_validation
--- PASS: TestAccGithubUtilRole_validation (0.00s)
=== RUN   TestAccGithubUtilTwoPartID
--- PASS: TestAccGithubUtilTwoPartID (0.00s)
=== RUN   TestAccValidateTwoPartID
=== RUN   TestAccValidateTwoPartID/valid
=== RUN   TestAccValidateTwoPartID/blank_ID
=== RUN   TestAccValidateTwoPartID/not_enough_parts
=== RUN   TestAccValidateTwoPartID/too_many_parts
--- PASS: TestAccValidateTwoPartID (0.00s)
    --- PASS: TestAccValidateTwoPartID/valid (0.00s)
    --- PASS: TestAccValidateTwoPartID/blank_ID (0.00s)
    --- PASS: TestAccValidateTwoPartID/not_enough_parts (0.00s)
    --- PASS: TestAccValidateTwoPartID/too_many_parts (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-github/github	78.084s

@paultyng paultyng merged commit 95a97c2 into integrations:master Jun 1, 2018
@chandy
Copy link

chandy commented Jun 1, 2018

Thanks all!

@endorama
Copy link
Contributor Author

endorama commented Jun 1, 2018

@paultyng thank you for the merge.

I wasn't able to step in before today, but I really appreciated your review; I'm sorry I didn't updated the code in time.

Thanks!

@endorama endorama deleted the add-repository-topics branch June 29, 2018 09:20
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants