Add github_branch data and resource #364

cornfeedhobo · 2020-02-25T04:38:24Z

This PR adds a new resource for creating and managing Github repository branches. I find this useful for modules that codify Gitflow.

Please let me know if I can fix anything.

$ TF_ACC=1 go test github.com/terraform-providers/terraform-provider-github/github \
    -v -timeout 120m -run=TestAccGithubBranchDataSource_basic
=== RUN   TestAccGithubBranchDataSource_basic
=== PAUSE TestAccGithubBranchDataSource_basic
=== CONT  TestAccGithubBranchDataSource_basic
--- PASS: TestAccGithubBranchDataSource_basic (9.28s)
PASS
ok      github.com/terraform-providers/terraform-provider-github/github (cached)

$ TF_ACC=1 go test github.com/terraform-providers/terraform-provider-github/github \
    -v -timeout 120m -run=TestAccGithubBranch_basic
=== RUN   TestAccGithubBranch_basic
=== PAUSE TestAccGithubBranch_basic
=== CONT  TestAccGithubBranch_basic
--- PASS: TestAccGithubBranch_basic (10.20s)
PASS
ok      github.com/terraform-providers/terraform-provider-github/github 10.210s

$ TF_ACC=1 go test github.com/terraform-providers/terraform-provider-github/github \
    -v -timeout 120m -run=TestAccGithubBranch_withSourceBranch
=== RUN   TestAccGithubBranch_withSourceBranch
=== PAUSE TestAccGithubBranch_withSourceBranch
=== CONT  TestAccGithubBranch_withSourceBranch
--- PASS: TestAccGithubBranch_withSourceBranch (6.91s)
PASS
ok      github.com/terraform-providers/terraform-provider-github/github 6.919s

cornfeedhobo · 2020-02-25T04:42:52Z

Partially addresses #286

shoekstra

Hey @cornfeedhobo,

This looks great! 👍 I gave some minor feedback...

Something that may need more discussion is the name of the resource. I think github_repository_branch is quite lengthy and github_branch would suffice (I think it's obvious github_branch would refer to a repo's branch).

Maybe @jcudit can weigh in here?

Stephen

github/data_source_github_repository_branch.go

github/resource_github_repository_branch.go

github/resource_github_repository_branch_test.go

github/data_source_github_repository_branch_test.go

cornfeedhobo · 2020-03-04T00:24:50Z

@shoekstra Thanks for the review. I left some questions for you to clarify, but in general I'm happy to change anything you want. All your points are minor, including the resource name. github_branch is fine with me. Let me know what you think or if you want to wait for @jcudit before I make any follow up changes.

Regards

…use required fixtures

cornfeedhobo · 2020-03-25T17:00:54Z

@shoekstra @jcudit

Bump. I addressed all the concerns I think. 🙏

jcudit · 2020-03-25T17:22:46Z

👋 @cornfeedhobo - this is looking good so far. I think all that is missing is to update the website/ directory with changes similar to these two previous examples:

Docs for both the datasource and resource would be helpful.

Also, I am in favour of github_repository_branch because it feels more organized and less ambiguous.

shoekstra · 2020-03-26T10:53:40Z

Sincere apologies for the delayed response! I haven't had time to look at the test errors, I'll see if I can do that later today...

Also, I am in favour of github_repository_branch because it feels more organized and less ambiguous.

I worry this leads to bloated resource names and we should only include repository in the resource name if there is also a non repository version of the resource, and we already have github_branch_protection, so github_branch feels a better fit than github_repository_branch.

But I don't want to let a naming discussion block this functionality, so I'll create a separate issue to discuss. 🙂

cornfeedhobo · 2020-03-27T05:55:05Z

I'm going to keep going with github_branch, but I do encourage a separate issue being opened about this, because while github_repository_branch* is verbose, I'm pretty sure that's a pill everyone has swallowed in the context of Terraform.

cornfeedhobo · 2020-03-30T21:04:51Z

I'm going to verify importing before signing off on this. stand by.

shoekstra · 2020-03-31T09:52:04Z

I'm going to verify importing before signing off on this. stand by.

You can add a test for the importer functionality in your resource's integration tests, here's an example.

cornfeedhobo · 2020-03-31T14:08:06Z

@shoekstra interesting. It looks like my code already does that, but then doing some functional tests and I see that after import, the subsequent apply is attempting to update most of the attributes. Is it possible the test suites should have a follow up apply step to check for drift after import?

shoekstra · 2020-03-31T15:04:38Z

This is generally solved by adding those attributes to the resource's Read func so when the resource is refreshed those values are read and then stored in the state.

You'll notice in resourceGithubRepositoryFileCreate for example that instead of setting values in the state file it calls resourceGithubRepositoryFileRead which then reads the updated resource and sets it's values in the state file. AFAIK you want to set as much as you can via the Read func so that changes are captured when refreshing or importing an existing resource.

Hope that helps!

cornfeedhobo · 2020-04-02T00:54:05Z

Fixes #401

cornfeedhobo · 2020-04-02T00:55:24Z

@jcudit @shoekstra thanks for hanging in there! I believe this is ready now.

cornfeedhobo · 2020-04-07T05:53:16Z

@shoekstra Thanks for the review. I think I've addressed everything you've brought up. It also gave me the chance to clean up some other minor things. Have another look please, and I'll try to be fast about any final changes. Thanks again!

shoekstra

Sorry for the delayed feedback @cornfeedhobo!

Some minor changes, fairly certain after this it'll be good to merge 👍

Ping @jcudit to also give it a look see.

github/resource_github_branch.go

github/resource_github_branch_test.go

website/docs/r/branch.html.markdown

cornfeedhobo · 2020-04-10T06:04:36Z

@shoekstra updated. thanks again.

shoekstra

Awesome!

jcudit · 2020-04-14T22:01:52Z

👋 I've got this one on my radar and am happy with the outcome so far. Great work 😄

I'm focusing on merging a client change that would cause a minor conflict here when merged. Afterwards, I'll give this branch a thorough review pass and will either get it merged with the upcoming release or the one that follows. Thanks again for the contribution and patience.

cornfeedhobo · 2020-04-15T02:31:00Z

Thanks! We have some work that is probably 2 weeks out, and it would be great to have this in by then.

jcudit

I've made a bunch of suggestions that will get the acceptance tests to run. Please commit the fixes if they look good.

This test consistently fails:

=== CONT  TestAccGithubBranch_withSourceBranch
--- FAIL: TestAccGithubBranch_withSourceBranch (6.59s)
    testing.go:654: Step 2 error: Error querying GitHub branch reference terraformtesting/test-repo (refs/heads/test-branch-jcduy:test-branch): no match found for this ref
FAIL
FAIL	github.com/terraform-providers/terraform-provider-github/github	6.965s

Still investigating but appreciate help if anything comes to mind.

github/data_source_github_branch.go

jcudit · 2020-04-24T19:01:34Z

github/data_source_github_branch_test.go

+			{
+				Config: testAccCheckGithubBranchDataSourceConfig(name, repo, "master"),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(rn, "repository", repo),
+					resource.TestCheckResourceAttr(rn, "branch", "master"),
+					resource.TestCheckResourceAttrSet(rn, "etag"),
+					resource.TestCheckResourceAttrSet(rn, "ref"),
+					resource.TestCheckResourceAttrSet(rn, "sha"),
+				),
+			},


Is this a duplicate of the block above?

@jcudit Yes. My understanding is that this checks for drift, essentially making sure my state doesn't accidentally drift between runs. This helped me catch an error with where I was setting etag.

jcudit · 2020-04-24T19:01:46Z

github/data_source_github_branch_test.go

+				Config: testAccCheckGithubBranchDataSourceConfig(name, repo, "test-branch"),
+				Check: resource.ComposeTestCheckFunc(
+					resource.TestCheckResourceAttr(rn, "repository", repo),
+					resource.TestCheckResourceAttr(rn, "branch", "test-branch"),
+					resource.TestCheckResourceAttrSet(rn, "etag"),
+					resource.TestCheckResourceAttrSet(rn, "ref"),
+					resource.TestCheckResourceAttrSet(rn, "sha"),
+				),
+			},


Is this a duplicate of the block above?

same answer as above.

github/provider.go

github/resource_github_branch.go

jcudit · 2020-04-24T19:10:33Z

github/resource_github_branch_test.go

+				ImportStateId:     fmt.Sprintf("%s:%s", repo, branch),
+				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{
+					"source_sha",


What does work around?

source_sha is extremely difficult to determine at import time. I dug through github's api and git. The best I figured out was an in-memory shallow clone and then parse the git state, but decided that was too much. It's just non-trivial to find the parent commit, and even once you do, it's not guaranteed accurate. It my testing I was getting

testing.go:654: Step 2 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected. (map[string]string) { } (map[string]string) (len=1) { (string) (len=10) "source_sha": (string) (len=40) "18edb0e04930c3e4cff23fb837d3116898e0c6a6" }

I assume this is because the way the tests are orchestrated because this new resource does not set it outside of Create.

github/resource_github_branch_test.go

website/docs/d/branch.html.markdown

github/resource_github_branch_test.go

cornfeedhobo · 2020-04-24T23:09:38Z

@jcudit The acceptance tests already ran here and locally - everything was fine. Did something change in master necessitating these? What's up - I'm lost.

jcudit · 2020-04-24T23:44:29Z

Sorry for the confusion @cornfeedhobo, there were changes merged around client and pointer use. These are the minor conflicts mentioned above.

Not sure about the failing test as yet.

cornfeedhobo · 2020-04-24T23:55:59Z

Gotchya. On it!

cornfeedhobo · 2020-04-25T20:58:43Z

@jcudit I've addressed most everything and left a comment on the import state question.

Unfortunately it looks like your travis config is broken right now. Ping me with your review and I'll kick off travis with an empty commit if all is good. Thanks!

jcudit · 2020-04-30T12:59:43Z

@cornfeedhobo - got it sorted, apologies for that interrupt. Can you try a rebase so we can validate tests are passing?

cornfeedhobo · 2020-04-30T21:45:34Z

@jcudit Look at that happy travis! thanks!

P.S. Can you Squash Merge this when the time comes?

jcudit · 2020-05-01T12:19:58Z

Happy to see this one ship! Great feature, great collaboration and a lot of patience to get to this point. Thanks again for your efforts.

cornfeedhobo · 2020-05-02T03:23:12Z

Likewise. Thanks for the thoughtful reviews!

* add github_repository_branch data and resource * rename github_repository_branch to github_branch and adjust tests to use required fixtures * nit * Trigger travis * Trigger travis * add github_branch documentation * revert go.sum change. * remove sha attribute * fix importing * include doc links * fix broken doc link * clean up github_branch: - add etag and sha attributes back - make errors more verbose for easier debugging - expand test case - require branch for data source * review * review * review * fix client references for github v3 client * fix util call * nit * fix import id when specifying source_branch

add github_repository_branch data and resource

29c18ba

ghost added the size/XL label Feb 25, 2020

shoekstra suggested changes Mar 3, 2020

View reviewed changes

rename github_repository_branch to github_branch and adjust tests to …

7a77eb4

…use required fixtures

cornfeedhobo changed the title ~~Add github_repository_branch data and resource~~ Add github_branch data and resource Mar 4, 2020

cornfeedhobo added 4 commits March 4, 2020 13:13

nit

09505f2

Trigger travis

dd1cb11

Trigger travis

3f9891f

Merge branch 'master' into add-github_repository_branch

793f674

add github_branch documentation

34759ab

ghost added the Type: Documentation Improvements or additions to documentation label Mar 27, 2020

revert go.sum change.

399eaac

jcudit added this to the v2.6.0 milestone Mar 30, 2020

cornfeedhobo added 5 commits April 1, 2020 18:58

remove sha attribute

5fa3cbb

fix importing

9a856d8

Merge branch 'master' into add-github_repository_branch

b826d09

include doc links

a26f674

fix broken doc link

fea50d9

shoekstra suggested changes Apr 9, 2020

View reviewed changes

github/resource_github_branch.go Outdated Show resolved Hide resolved

github/resource_github_branch.go Show resolved Hide resolved

github/resource_github_branch_test.go Show resolved Hide resolved

website/docs/r/branch.html.markdown Show resolved Hide resolved

cornfeedhobo added 3 commits April 10, 2020 00:44

review

d2fc48e

review

89b1e1e

review

a3e87a0

shoekstra approved these changes Apr 10, 2020

View reviewed changes

jcudit modified the milestones: v2.8.0, v2.7.0 Apr 14, 2020

jcudit reviewed Apr 24, 2020

View reviewed changes

cornfeedhobo added 4 commits April 25, 2020 15:39

Merge branch 'master' into add-github_repository_branch

57fb500

fix client references for github v3 client

15abf85

fix util call

1b768ac

nit

12be2ab

jcudit mentioned this pull request Apr 27, 2020

Fix CI Failure For make tools #439

Closed

cornfeedhobo added 2 commits April 30, 2020 12:22

Merge branch 'master' into add-github_repository_branch

6550ac2

fix import id when specifying source_branch

62fcc4d

jcudit merged commit 0544e54 into integrations:master May 1, 2020

jcudit mentioned this pull request Dec 8, 2020

[Feature Request] Pull Requests and git branch publishes #286

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add github_branch data and resource #364

Add github_branch data and resource #364

cornfeedhobo commented Feb 25, 2020 •

edited

Loading

cornfeedhobo commented Feb 25, 2020

shoekstra left a comment

cornfeedhobo commented Mar 4, 2020

cornfeedhobo commented Mar 25, 2020

jcudit commented Mar 25, 2020

shoekstra commented Mar 26, 2020

cornfeedhobo commented Mar 27, 2020 •

edited

Loading

cornfeedhobo commented Mar 30, 2020

shoekstra commented Mar 31, 2020

cornfeedhobo commented Mar 31, 2020 •

edited

Loading

shoekstra commented Mar 31, 2020

cornfeedhobo commented Apr 2, 2020

cornfeedhobo commented Apr 2, 2020

cornfeedhobo commented Apr 7, 2020

shoekstra left a comment

cornfeedhobo commented Apr 10, 2020 •

edited

Loading

shoekstra left a comment

jcudit commented Apr 14, 2020

cornfeedhobo commented Apr 15, 2020

jcudit left a comment

jcudit Apr 24, 2020

cornfeedhobo Apr 25, 2020

jcudit Apr 24, 2020

cornfeedhobo Apr 25, 2020

jcudit Apr 24, 2020

cornfeedhobo Apr 25, 2020 •

edited

Loading

cornfeedhobo commented Apr 24, 2020

jcudit commented Apr 24, 2020

cornfeedhobo commented Apr 24, 2020

cornfeedhobo commented Apr 25, 2020 •

edited

Loading

jcudit commented Apr 30, 2020

cornfeedhobo commented Apr 30, 2020 •

edited

Loading

jcudit commented May 1, 2020

cornfeedhobo commented May 2, 2020

Add github_branch data and resource #364

Add github_branch data and resource #364

Conversation

cornfeedhobo commented Feb 25, 2020 • edited Loading

cornfeedhobo commented Feb 25, 2020

shoekstra left a comment

Choose a reason for hiding this comment

cornfeedhobo commented Mar 4, 2020

cornfeedhobo commented Mar 25, 2020

jcudit commented Mar 25, 2020

shoekstra commented Mar 26, 2020

cornfeedhobo commented Mar 27, 2020 • edited Loading

cornfeedhobo commented Mar 30, 2020

shoekstra commented Mar 31, 2020

cornfeedhobo commented Mar 31, 2020 • edited Loading

shoekstra commented Mar 31, 2020

cornfeedhobo commented Apr 2, 2020

cornfeedhobo commented Apr 2, 2020

cornfeedhobo commented Apr 7, 2020

shoekstra left a comment

Choose a reason for hiding this comment

cornfeedhobo commented Apr 10, 2020 • edited Loading

shoekstra left a comment

Choose a reason for hiding this comment

jcudit commented Apr 14, 2020

cornfeedhobo commented Apr 15, 2020

jcudit left a comment

Choose a reason for hiding this comment

jcudit Apr 24, 2020

Choose a reason for hiding this comment

cornfeedhobo Apr 25, 2020

Choose a reason for hiding this comment

jcudit Apr 24, 2020

Choose a reason for hiding this comment

cornfeedhobo Apr 25, 2020

Choose a reason for hiding this comment

jcudit Apr 24, 2020

Choose a reason for hiding this comment

cornfeedhobo Apr 25, 2020 • edited Loading

Choose a reason for hiding this comment

cornfeedhobo commented Apr 24, 2020

jcudit commented Apr 24, 2020

cornfeedhobo commented Apr 24, 2020

cornfeedhobo commented Apr 25, 2020 • edited Loading

jcudit commented Apr 30, 2020

cornfeedhobo commented Apr 30, 2020 • edited Loading

jcudit commented May 1, 2020

cornfeedhobo commented May 2, 2020

cornfeedhobo commented Feb 25, 2020 •

edited

Loading

cornfeedhobo commented Mar 27, 2020 •

edited

Loading

cornfeedhobo commented Mar 31, 2020 •

edited

Loading

cornfeedhobo commented Apr 10, 2020 •

edited

Loading

cornfeedhobo Apr 25, 2020 •

edited

Loading

cornfeedhobo commented Apr 25, 2020 •

edited

Loading

cornfeedhobo commented Apr 30, 2020 •

edited

Loading