terraform-provider-github icon indicating copy to clipboard operation
terraform-provider-github copied to clipboard

Refactor out branches attribute to new data source

Open k24dizzle opened this issue 2 years ago • 5 comments

  • Refactor out branches attribute from resource_github_repository and data_source_github_repository
  • Introduce a new data_source called data_source_github_repository_branches that returns branches instead.

Rationale: I believe branches shouldn't belong to the attributes of the github_repository resource/data source. These fields should be relevant to the repository itself, not other auxiliary resources (like teams, collaborators, branches, etc.)

Similar to github_collaborators, github_repository_pull_requests, and other data sources, it would be better to create a new data source to contain this information instead.

If a developer wants to interact with the branches of a repo, they can use data_source_github_repository_branches instead. This helps prevent some of the problems described in the issues below.

Testing done in this repo: https://github.com/kzhaotest/branches

Fixes: https://github.com/integrations/terraform-provider-github/issues/1037 https://github.com/integrations/terraform-provider-github/issues/1010

While still supporting the functionality of: https://github.com/integrations/terraform-provider-github/issues/748

k24dizzle avatar Apr 18 '22 23:04 k24dizzle

Hi @kfcampbell, I'm curious if there's any thoughts on this/best ways to roll this out

k24dizzle avatar May 03 '22 18:05 k24dizzle

@kfcampbell friendly bump

k24dizzle avatar May 31 '22 18:05 k24dizzle

@k24dizzle apologies for the slow response time. I'll shoot to get this into the next release!

kfcampbell avatar Jun 10 '22 23:06 kfcampbell

@kfcampbell Any update on getting this into a release? It looks like the 4.27.0 milestone was missed by the release.

eriksw avatar Jul 20 '22 17:07 eriksw

@kfcampbell it would be great to get this out ASAP if you can squeeze it in.

Thanks.

DoctorPolski avatar Aug 05 '22 10:08 DoctorPolski

@k24dizzle Apologies for the delay. Integration tests on this aren't passing for me:

--- FAIL: TestAccGithubBranchesDataSource (83.24s)
    --- FAIL: TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository (80.94s)
        --- SKIP: TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository/with_an_individual_account (0.00s)
        --- FAIL: TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository/with_an_organization_account (79.90s)
FAIL
The error seems to be a result of this snippet:
testing.go:654: Step 0 error: After applying this step, the plan was not empty:
    DIFF:
    
    UPDATE: data.github_repository_branches.test
      branches:   "" => "<computed>"
      id:         "" => "<computed>"
      repository: "" => "tf-acc-test-branches-2wdmv"
    UPDATE: github_repository.test
      allow_auto_merge:       "false" => "false"
      allow_merge_commit:     "true" => "true"
      allow_rebase_merge:     "true" => "true"
      allow_squash_merge:     "true" => "true"
      archived:               "false" => "false"
      auto_init:              "true" => "true"
      default_branch:         "main" => "main"
      delete_branch_on_merge: "false" => "false"
      description:            "" => ""
      etag:                   "W/\"b693979949e92b7a9a6fe1a72dfb591c806dbf206830fb62131e9b10b6419376\"" => "W/\"b693979949e92b7a9a6fe1a72dfb591c806dbf206830fb62131e9b10b6419376\""
      full_name:              "kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv" => "kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv"
      git_clone_url:          "git://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv.git" => "git://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv.git"
      has_downloads:          "false" => "false"
      has_issues:             "false" => "false"
      has_projects:           "false" => "false"
      has_wiki:               "false" => "false"
      homepage_url:           "" => ""
      html_url:               "https://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv" => "https://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv"
      http_clone_url:         "https://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv.git" => "https://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv.git"
      id:                     "tf-acc-test-branches-2wdmv" => "tf-acc-test-branches-2wdmv"
      is_template:            "false" => "false"
      name:                   "tf-acc-test-branches-2wdmv" => "tf-acc-test-branches-2wdmv"
      node_id:                "R_kgDOH46FUQ" => "R_kgDOH46FUQ"
      pages.#:                "0" => "0"
      private:                "false" => "false"
      repo_id:                "529433937" => "529433937"
      ssh_clone_url:          "[email protected]:kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv.git" => "[email protected]:kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv.git"
      svn_url:                "https://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv" => "https://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv"
      template.#:             "0" => "0"
      visibility:             "public" => "public"
      vulnerability_alerts:   "true" => ""
    
    
    
    STATE:
    
    data.github_repository_branches.test:
      ID = kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv
      provider = provider.github
      branches.# = 1
      branches.0.name = main
      branches.0.protected = false
      repository = tf-acc-test-branches-2wdmv
    
      Dependencies:
        github_repository.test
    github_repository.test:
      ID = tf-acc-test-branches-2wdmv
      provider = provider.github
      allow_auto_merge = false
      allow_merge_commit = true
      allow_rebase_merge = true
      allow_squash_merge = true
      archived = false
      auto_init = true
      default_branch = main
      delete_branch_on_merge = false
      description = 
      etag = W/"b693979949e92b7a9a6fe1a72dfb591c806dbf206830fb62131e9b10b6419376"
      full_name = kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv
      git_clone_url = git://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv.git
      has_downloads = false
      has_issues = false
      has_projects = false
      has_wiki = false
      homepage_url = 
      html_url = https://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv
      http_clone_url = https://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv.git
      is_template = false
      name = tf-acc-test-branches-2wdmv
      node_id = R_kgDOH46FUQ
      private = false
      repo_id = 529433937
      ssh_clone_url = [email protected]:kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv.git
      svn_url = https://github.com/kfcampbell-terraform-provider/tf-acc-test-branches-2wdmv
      visibility = public
      vulnerability_alerts = true

Are the tests passing for you locally?

kfcampbell avatar Aug 27 '22 00:08 kfcampbell

@kfcampbell The TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository/with_an_organization_account test is passing for me.

$ export GITHUB_ORGANIZATION=test-org-mareksapota-lyft
$ export GITHUB_OWNER=mareksapota-lyft
$ go clean -testcache
$ TF_ACC=1 go test -v -count=1 ./... -run '^TestAccGithubBranchesDataSource'
?       github.com/integrations/terraform-provider-github/v4    [no test files]
=== RUN   TestAccGithubBranchesDataSource
=== RUN   TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository
=== RUN   TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository/with_an_anonymous_account
    data_source_github_repository_branches_test.go:46: anonymous account not supported for this operation
=== RUN   TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository/with_an_individual_account
=== RUN   TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository/with_an_organization_account
--- PASS: TestAccGithubBranchesDataSource (13.99s)
    --- PASS: TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository (13.99s)
        --- SKIP: TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository/with_an_anonymous_account (0.00s)
        --- PASS: TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository/with_an_individual_account (7.22s)
        --- PASS: TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository/with_an_organization_account (6.77s)
PASS
ok      github.com/integrations/terraform-provider-github/v4/github     14.358s

mareksapota-lyft avatar Sep 01 '22 23:09 mareksapota-lyft

@kfcampbell The TestAccGithubBranchesDataSource/manages_branches_of_a_new_repository/with_an_organization_account test is passing for me.

On both f857f8ff9e1f2742d2af5bf26918992e28ec8407 and 2b683f64da3345452f915e9cbeed262472eaa6db.

mareksapota-lyft avatar Sep 01 '22 23:09 mareksapota-lyft

@kfcampbell went ahead and updated the branch with the upstream ^

the tests work for me locally as well:

~/go/src/github.com/terraform-provider-github [branches] % go test -v ./... -run ^TestAccGithubRepositoryBranchesDataSource         
?   	github.com/integrations/terraform-provider-github/v4	[no test files]
=== RUN   TestAccGithubRepositoryBranchesDataSource
=== RUN   TestAccGithubRepositoryBranchesDataSource/manages_branches_of_a_new_repository
=== RUN   TestAccGithubRepositoryBranchesDataSource/manages_branches_of_a_new_repository/with_an_anonymous_account
    data_source_github_repository_branches_test.go:46: anonymous account not supported for this operation
=== RUN   TestAccGithubRepositoryBranchesDataSource/manages_branches_of_a_new_repository/with_an_individual_account
    provider_utils.go:56: GITHUB_TOKEN and GITHUB_OWNER environment variables should be set
    provider_utils.go:66: Skipping TestAccGithubRepositoryBranchesDataSource/manages_branches_of_a_new_repository/with_an_individual_account which requires individual mode
=== RUN   TestAccGithubRepositoryBranchesDataSource/manages_branches_of_a_new_repository/with_an_organization_account
--- PASS: TestAccGithubRepositoryBranchesDataSource (8.38s)
    --- PASS: TestAccGithubRepositoryBranchesDataSource/manages_branches_of_a_new_repository (8.38s)
        --- SKIP: TestAccGithubRepositoryBranchesDataSource/manages_branches_of_a_new_repository/with_an_anonymous_account (0.00s)
        --- SKIP: TestAccGithubRepositoryBranchesDataSource/manages_branches_of_a_new_repository/with_an_individual_account (0.00s)
        --- PASS: TestAccGithubRepositoryBranchesDataSource/manages_branches_of_a_new_repository/with_an_organization_account (8.38s)
PASS
ok  	github.com/integrations/terraform-provider-github/v4/github	8.641s

If I'm interpreting the failure correctly, it seems like the provider thinks the repository resource needs to be updated. (My data source is irrelevant to the failure as it should not be a part of the plan b/c it is not a resource)

vulnerability_alerts:   "true" => ""

This seems like the only field the plan wants to change in the repo resource. This PR seems relevant, but still not sure why it works for us but not for you... https://github.com/integrations/terraform-provider-github/pull/768

k24dizzle avatar Sep 01 '22 23:09 k24dizzle

If dependabot is enabled on the organization by default tests will fail. Tests pass with this setting off:

Screen Shot 2022-09-01 at 4 25 49 PM

https://github.com/organizations/ORGNAME/settings/security_analysis

mareksapota-lyft avatar Sep 01 '22 23:09 mareksapota-lyft