go-github update

[andrew-waters]

https://github.com/google/go-github/releases/tag/v32.0.0 has been released and included breaking changes to a few functions in this provider. This PR aims to maintain parity with existing functionality but will unblock work on organisation secrets (https://github.com/terraform-providers/terraform-provider-github/issues/468)

[anGie44]

hi @andrew-waters thank you for this PR!
One thing I did notice which I believe is a result of these upstream changes here google/go-github@f093974 is that the BranchProtection restriction-related tests are failing:

   TestAccGithubBranchProtection_teams: testing.go:654: Step 0 error: errors during apply:

        Error: PUT https://api.github.com/repos/github-test-org/tf-acc-test-branch-prot-v6opn/branches/master/protection: 422 Invalid request.

        No subschema in "anyOf" matched.
        "teams", "users" weren't supplied.
        Not all subschemas of "allOf" matched.
        For 'anyOf/1', {} is not a null. []

          on /var/folders/0l/3rqflfpd06z9qmklfcvqy1yh0000gn/T/tf-test942841618/main.tf line 28:
          (source code not available)

  TestAccGithubBranchProtection_basic: testing.go:654: Step 0 error: errors during apply:

        Error: PUT https://api.github.com/repos/github-test-org/tf-acc-test-branch-prot-q10kx/branches/master/protection: 422 Invalid request.

        No subschema in "anyOf" matched.
        "teams" wasn't supplied.
        Not all subschemas of "allOf" matched.
        For 'anyOf/1', {"users"=>["anGie44"]} is not a null. []

          on /var/folders/0l/3rqflfpd06z9qmklfcvqy1yh0000gn/T/tf-test210218632/main.tf line 8:
          (source code not available)

 TestAccGithubBranchProtection_users: testing.go:647: Step 0, expected error:

        errors during apply: PUT https://api.github.com/repos/github-test-org/tf-acc-test-branch-prot-or9u7/branches/master/protection: 422 Invalid request.

        No subschema in "anyOf" matched.
        "teams" wasn't supplied.
        Not all subschemas of "allOf" matched.
        For 'anyOf/1', {"users"=>["user_with_underscore"]} is not a null. []

        To match:

        unable to add users in restrictions: user_with_underscore

    TestAccGithubBranchProtection_emptyItems: testing.go:654: Step 0 error: errors during apply:

        Error: PUT https://api.github.com/repos/github-test-org-ap/tf-acc-test-branch-prot-49dp8/branches/master/protection: 422 Invalid request.

        No subschema in "anyOf" matched.
        "teams", "users" weren't supplied.
        Not all subschemas of "allOf" matched.
        For 'anyOf/1', {} is not a null. []

          on /var/folders/0l/3rqflfpd06z9qmklfcvqy1yh0000gn/T/tf-test270873605/main.tf line 8:
          (source code not available)

We may need to adjust the implementation in the expansion of restrictions such that the empty fields (users, teams, and/or apps) are not necessarily passed as empty GoLang slices as they are right now with ([]string{}) as seen in this method https://github.com/terraform-providers/terraform-provider-github/blob/feda68b147d322ed95474811568292dad4273dd8/github/resource_github_branch_protection.go#L567 OR an upstream change might be upcoming if this is a regression in the library (I submitted an issue google/go-github#1546 to track this new behavior)

[anGie44]

we'll want to remove the previous go-github version

[andrew-waters]

@anGie44 apologies for the delay getting back to you here - I've just committed 79d396e to get rid of v31 completely.

The underlying changes you've created an issue for I'm also subscribed to so let's see what comes of that conversation - it doesn't appear to have been resolved.

[anGie44]

no worries @andrew-waters, thanks for having a look! reminds me I should check-in on that issue

[ejhayes]

It looks like the tests are currently passing with these changes. Is this something that can be merged in or are you still waiting on google/go-github#1546?

[hitmands]

Is there any update on this regards?

[andrew-waters]

I'd refer to @anGie44 - as she ran the tests that picked up the issue, it would be good to see if they now pass as is suggested

[anGie44]

hi @hitmands, @andrew-waters! Unfortunately without upstream changes in the go-github library, the acceptance tests discussed above are still failing (perhaps you were referring to the Travis CI checks @ejhayes?) good news is the PR to revert that breakage is in review google/go-github#1571 and once that's in we can pick this one back up 👍

[andrew-waters]

@anGie44 as you're already aware google/go-github#1571 has now been merged. Let me know if this PR needs any further work

[andrew-waters]

I've upgraded the lib to the newly released v32.1.0.

I'd be very grateful if someone can run the acceptance tests to ensure that the release has fixed the issue we faced.

[anGie44]

LGTM @andrew-waters 👍

With the exception of TestAccGithubRepositoryFile tests failing unrelated to these changes (pending PR #491), all tests are passing for the provider authenticated with organization-level owners. For individual account owners, all tests pass/skip as expected with the exception of TestAccGithubActionsSecret_disappears which is failing but is unrelated to the changes here as well .

[ejhayes]

@andrew-waters looks like this can be merged in any time.

[andrew-waters]

Can I self merge @ejhayes ? First contribution :)

[ejhayes]

Good point @andrew-waters. @anGie44 are you able to merge this change in?

[ejhayes]

@hitmands are you able to merge this PR?

[nikolay]

@hitmands Any chance to merge this?

[ejhayes]

@jcudit are you able to merge this PR?

[jcudit]

@ejhayes yes, but in time. This change and the feature it unblocks is tracked to be released at the end of September. Focus is currently on cleaning up testing and releasing v3.0.0. Once that blocker is cleared, we can get back to shipping changes like this at a more frequent pace. 🙇 for the patience on this one.

[jcudit]

Resolving conflicts seems to have broken the tests in some way 🙃 . I've merged b587cb0 to accomplish the upgrade instead.

[andrew-waters]

Thanks for adding this feature @jcudit. I'm glad that the proposed functionality can be achieved now as it is functionality I can leverage.

I'm somewhat disappointed that this PR had to be dropped - It seems that a lack of urgency (the PR was first cut on May 28th) turned into an abundance of urgency (the merge took less than 24 hours with a fresh cut) and I didn't get the opportunity to resolve them myself.

[jcudit]

Apologies @andrew-waters, fair concern.

I have been working through a transition with primary responsibilities. Time has been sparse for this project and I've been more aggressive with changes to compensate.

I expect us to get a few more maintainers and return to a more predictable pace in the coming months.