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

github_branch_default: 422 Unprocessable Entity #625

Closed
dee-kryvenko opened this issue Dec 7, 2020 · 10 comments
Closed

github_branch_default: 422 Unprocessable Entity #625

dee-kryvenko opened this issue Dec 7, 2020 · 10 comments
Labels
r/branch_default Type: Bug Something isn't working as documented

Comments

@dee-kryvenko
Copy link
Contributor

Terraform Version

{
  "terraform_version": "0.14.0",
  "terraform_revision": "",
  "provider_selections": {
    "registry.terraform.io/hashicorp/github": "4.1.0"
  },
  "terraform_outdated": false
}

Affected Resource(s)

  • github_branch_default

Terraform Configuration Files

resource "github_branch_default" "this" {
  repository = github_repository.this.name
  branch     = github_branch.this.branch
}

Debug Output

https://gist.github.com/dee-kryvenko/7cc5b517ef72ce4f87ee63aec5d178b0

Expected Behavior

Should have set default branch successfully.

Actual Behavior

Fails to do so.

Steps to Reproduce

  1. terraform apply

Important Factoids

I managed to narrow it down via curl to the following header: Accept: application/vnd.github.baptiste-preview+json, application/vnd.github.nebula-preview+json. It seems like if I remove application/vnd.github.nebula-preview+json part from my headers - it works. As it is used for the visibility field as described here https://docs.github.com/en/free-pro-team@latest/rest/reference/repos, I have tried to experiment with this field and it seems to only work if I set visibility: public. It fails as described above for both private and internal - so, unable to reproduce on a free org. It might be an issue with GitHub - I am currently talking to the Enterprise support and will update here with their findings.

References

@javed7891
Copy link

I am facing this same issue & also the one mentioned in #620.

@jcudit jcudit added Type: Bug Something isn't working as documented r/branch_default labels Dec 23, 2020
@Gowiem
Copy link

Gowiem commented Jan 5, 2021

I have had a project run into this as well as soon as we upgraded and started using this resource. Would be interested in the fix for this awesome new resource!

@dee-kryvenko
Copy link
Contributor Author

To be honest I do not believe there's any issues with a resource at this point. This really looks like a GitHub API bug. I just created this ticket so that we have a place to communicate. GitHub Enterprise support still haven't got back to me even though I ping them weekly. They say they are working on reproducing the issue (it's a simple curl request, c'mon). Maybe they are awaiting for more signals from the community that it is a wide spread issue - I don't know.

@Gowiem
Copy link

Gowiem commented Jan 5, 2021

@dee-kryvenko ah interesting. Thanks for sharing the context and good luck with hashing it out with them!

@timmehhh7
Copy link

Facing this as well... Can't modify the default branch for private repos. @dee-kryvenko if there is an issue you can reference folks can maybe weigh in there as well to help GH understand how wide-spread it is?

Thanks!
Tim

@dee-kryvenko
Copy link
Contributor Author

I'm talking to them via their Enterprise support plan through my employer. I don't think these tickets are public. There's no updates on this BTW.
Maybe we should look for ways to reach out to them publicly. Any ideas on what that might be?

@dee-kryvenko
Copy link
Contributor Author

I finally got an update from GitHub support. Seems that it is indeed a bug in the provider, though the error message returned by API is a little bit off but more on that later.

It seems like this provider currently sends the whole repository JSON as a body to the PATCH request. It turns out it supposed to only be sending fields that did changed, i.e. for github_branch_default resource particularly the request should look like the following:

curl https://api.github.com/repos/xxx/yyy \
  -u username:${GITHUB_TOKEN} \
  -X PATCH \
  -H 'Accept: application/vnd.github.baptiste-preview+json, application/vnd.github.nebula-preview+json' \
  -d '{"default_branch": "master"}'

I tested and this request indeed works fine, even though it includes application/vnd.github.nebula-preview+json part in the Accept header. Changing visibility field works that way too.

It seems like this provider needs to be doing a better job in terms of identifying which fields did change and be sending only that changed fields.

It seems like sending everything worked fine before, but something is different about this new visibility field and the header that enables it. It seems like a combination of application/vnd.github.nebula-preview+json presence along with unchanged visibility field in case it was either private or internal but not public in the request triggers this error, which sounds like a bug or at the very least a backward incompatible change, and the error message it returns doesn't makes any sense. I've sent two additional questions to the GitHub rep:

  1. It seems like before introducing that visibility field along with that application/vnd.github.nebula-preview+json header - the API worked fine even when the whole mostly-unchanged repository object JSON was sent to the PATCH endpoint. Even though that behavior wasn't explicitly documented, the opposite wasn't documented either. I would imagine GitHub Terraform provider is not the only one using the API that way, so depends on the point of view it might or might not be considered a backward incompatible change. Could you clarify please what's the official GitHub stance on that, and will that be explicitly documented as not supported scenario or maybe your team would consider adding this as a new supported scenario?

  2. The error message it returns ("The branch master was not found. Please push that ref first or create it via the Git Data API.") is definitely wrong. The branch did existed and the message didn't help at all with the troubleshooting. I don't think without this support ticket anyone would been able to figure this out, especially that the documentation doesn't say explicitly that I can't send unchanged fields back to the API. I think it definitely needs fixing.

I'll pass along their feedback when I get it, regardless now we know that the issue is on the provider side and we know what needs to be fixed.

The code for github_branch_default seems to be pretty straight forward briefly looking into it. Sounds like we just need to remove Get call before calling Edit and ad-hoc creating a body object instead. That should also probably fix #620. I might be able to send a PR fixing it shortly. But it sounded like we have more places broken that way, if so - that will not automatically fix these.

dee-kryvenko added a commit to dee-kryvenko/terraform-provider-github that referenced this issue Jan 21, 2021
@dee-kryvenko
Copy link
Contributor Author

Alright, unless I'm missing something - that should be the fix #666
Love the 666 number of it.

@dee-kryvenko
Copy link
Contributor Author

@jcudit would you be able please to take a look?

@jcudit
Copy link
Contributor

jcudit commented Jan 22, 2021

Will do! Thanks for your persistence here.

@jcudit jcudit closed this as completed in b61a71f Jan 22, 2021
k24dizzle added a commit to lyft/terraform-provider-github that referenced this issue Feb 22, 2021
* v4.1.0

* Fix unable to resolve node id for branch_protection (integrations#610)

* Don't check node id for length

* Check if node id is valid base 64

Co-authored-by: Willem Gillis <willem.gillis@imec.be>

* temporarily disable PR acceptance testing

these jobs all fail and are confusing to contributors when launched from 
a PR raised by a fork.  there are ways to get around this, but will 
defer until the repository is transferred.  disabling for now.

* remove `ForceNew` on `template*` as they are concerns only at creation time (integrations#609)

There are a number of resources that have been marked as `ForceNew: true` out of a desire in "correctness" by those that do not actually understand how these resources are used in the real world and damage that can be done. No one wants to blow up a repository to change something like this, if they need to there is a mechanism built into terraform called [taint](https://www.terraform.io/docs/commands/taint.html). While there are some things that make sense for using `ForceNew` a repository for source control on properties that the API will ignore outside of creation is not one of them.

Signed-off-by: Ben Abrams <me@benabrams.it>

* Add diff suppression function to the branch protection resource (integrations#614)

Adding a diff suppression function to the branch protection resource to
ignore the strict status check field if no contexts have been specified.

This resolves the issue with the GraphQL API returning a strict status
check value of "true" by default, regardless of contexts being set or
not.

* Add Apps to actor types in branch protection (integrations#615)

Adding Github Apps to actor types in the branch protection resource.

NOTE: Apps as an actor type is only available in push restrictions.

* Added `allowsDeletions`and `allowsForcePushes`settings (integrations#623)

* Added `allowsDeletions`and `allowsForcePushes`settings https://developer.github.com/v4/changelog/2020-11-13-schema-changes/ (#1)

* complete documentation

* update module github.com/shurcooL/githubv4 with `go get github.com/shurcooL/githubv4`

* vendor latest githubv4

* add test for deletions and force pushes

Co-authored-by: Jeremy Udit <jcudit@github.com>

* Fix syntax error

* Conditionally Run GHES Test Suite

* Run gofmt (integrations#645)

Signed-off-by: Stephen Hoekstra <shoekstra@schubergphilis.com>

* Allow dependabot to check github actions (integrations#643)

* Typo: s/visiblity/visibility (integrations#629)

Small typo in the docs.

* github_repository_webhook: describe content_type options (integrations#510)

* change private to visibility (integrations#635)

* Use commit SHA to lookup commit info in github_repository_file resource (integrations#644)

* Fix references to "master"

Signed-off-by: Stephen Hoekstra <shoekstra@schubergphilis.com>

* Use commit SHA to lookup commit info

Currently the provider loops through commits in a repo until it finds the most recent commit containing the managed file. This is inefficient and could lead to you being rate limited if managing a few files that were updated a long time ago.

This commit fixes that by storing the commit SHA when updating the file and using that SHA to lookup the commit info instead of looping through all commits.

Signed-off-by: Stephen Hoekstra <shoekstra@schubergphilis.com>

* add release automation for terraform registry

/cc https://www.terraform.io/docs/registry/providers/publishing.html

* Add v4.2.0 Release (integrations#641)

* add v4.1.1 release items

* correct semver version

* Document Additional Breaking Change For v3.0.0

* add `github_repository_file` bugfix

* move to correct release

* add goreleaser configuration to enable release automation

* resource/repository: add support for enabling github pages (integrations#490)

* add support for enabling github pages

* update resource comments

* add additional comments in expand methods

* add formatting fixes

Co-authored-by: Jeremy Udit <jcudit@github.com>

* Add `github_branch_protection_v3` Resource (integrations#642)

* Add `branch_protection_v3` Resource

- add new resource to `website/github.erb`
- add new resource to `website/docs/r/<resource>.html.markdown`
- add new resource to `github/provider.go`
- add tests for resource in `github/resource_<resource>_test.go`
- implement new resource in `github/resource_<resource>.go`

* fixup! gofmt fixes

* add changelog entries for v4.3.0 release (integrations#658)

* Remove github.com/hashicorp/terraform from dependencies (integrations#628)

* remove github.com/hashicorp/terraform from dependencies

* go mod tidy

* refactor: execute fmt

* Allow dependabot to check go dependencies (integrations#653)

* Fix link to Milestones page (integrations#663)

* github_branch_default: send only fields that changed. Fixes integrations#625 integrations#620. (integrations#666)

* github_branch_default: send only fields that changed. Fixes integrations#625 integrations#620.

* fix failing test and update docs

Co-authored-by: Jeremy Udit <jcudit@github.com>

* Fix error handling (integrations#668)

Do not silently proceed further on receiving an error response.

Signed-off-by: rustyclock <rustyclock@protonmail.com>

* Update CHANGELOG.md

* Remove Obsolete Test

My understanding is our use of Terraform Registry makes this failing test unnecessary.

* Update GitHub organization references (integrations#672)

Following the project transfer from `terraform-providers` organization to `integrations`.

* Add Example For `github_team_repository` (integrations#676)

* Add Example For `github_team_repository`

also documents a limitation with `for_each` in this scenario

* fixup! add newline

* changelog: manually fix links to issues (integrations#673)

They were pointing to the old archived repo, which doesn't have newer issues.

* Handle base64 decodable repo names (integrations#684)

* github/config: Fix detection of individual, non-org accounts (integrations#685)

- Previously, whenever an individual user tried to interact with their
  repos, the provider would return an error, rendering v4.3.1 unusable
  _for individuals_:

  ```
  ➜ terraform plan
  Error: GET https://api.github.com/orgs/issyl0: 404 Not Found []
  on /Users/issyl0/repos/terraform/github.tf line 1, in provider "github":
  ```

- `ConfigureOwner` works such that if the `owner.name` is not blank (ie,
  the user had specified `owner = <username>` in their Terraform file),
  the code progresses to check if the `owner.name` is an org.
  Importantly, that check (prior to this change) returned an error if
  `owner.name` was not an org. That final error meant it was impossible
  to run if not using an organisation account.

- Reproduction steps: https://gist.github.com/issyl0/cd61e4cb59de2c2e1e8f45e3cf7c12f5

* add changelog entry for v4.3.2

* Add `create_default_maintainer` Option To `github_team` (integrations#661)

* Add `create_default_maintainer` option to `github_team`

This adds a possible fix to the following issues by providing users with an option to remove the automatic addition of a default maintainer to a team during creation.

/cc integrations#527
/cc integrations#104
/cc integrations#130

* Refresh CONTRIBUTING Documentation (integrations#682)

* refresh contributing docs

* add quick instructions

* add updates for newer versions of terraform

* Add Diff Suppression Option To `repository_collaborator` (integrations#683)

* add diff suppression option to `repository_collaborator`

* fixup! remove comment

* remove hardcoded username from test

* Update CHANGELOG for v4.4.0 release

* Add repo context to error message when branch is not found (integrations#691)

* Add repo context to error message when branch is not found

* fix failing `TestAccGithubRepositoryFile` test

access committer data through `Commit` struct

Co-authored-by: Jeremy Udit <jcudit@github.com>

* fix based on linter (integrations#694)

* add v4.4.1 release notes

* Modify github_team_repository to accept slug as a valid team_id as well (integrations#693)

* First attempt

* Add comment

* Attempt to modify unit tests

* Edit docs to reflect change

* Make sure team_id is set appropriately

* fixing lint

* add passing tests for `github_team_repository`

Co-authored-by: Jeremy Udit <jcudit@github.com>

* add v4.5.0 release notes

* temporarily ignore `darwin/arm64` to unblock releases

/cc integrations#695

* update release notes for v4.5.0

Co-authored-by: tf-release-bot <terraform@hashicorp.com>
Co-authored-by: Polygens <willem.gillis@gmail.com>
Co-authored-by: Willem Gillis <willem.gillis@imec.be>
Co-authored-by: Jeremy Udit <jcudit@github.com>
Co-authored-by: Ben Abrams <me@benabrams.it>
Co-authored-by: Patrick Marabeas <patrick@marabeas.io>
Co-authored-by: Francois BAYART <francois.bayart@kensu.io>
Co-authored-by: Stephen Hoekstra <shoekstra@schubergphilis.com>
Co-authored-by: John Losito <lositojohnj@gmail.com>
Co-authored-by: Bret <166301+bcomnes@users.noreply.github.com>
Co-authored-by: Jakub Holy <jakubholy@jakubholy.net>
Co-authored-by: Ichinose Shogo <shogo82148@gmail.com>
Co-authored-by: angie pinilla <angelinepinilla@gmail.com>
Co-authored-by: Shu Kutsuzawa <cappyzawa@yahoo.ne.jp>
Co-authored-by: Oleksandr Dievri <o.dievri@gmail.com>
Co-authored-by: Dee Kryvenko <109895+dee-kryvenko@users.noreply.github.com>
Co-authored-by: Ravi <1299606+rustycl0ck@users.noreply.github.com>
Co-authored-by: Alexis Gauthiez <alexis.gauthiez@gmail.com>
Co-authored-by: Christian Höltje <docwhat@gerf.org>
Co-authored-by: Issy Long <me@issyl0.co.uk>
Co-authored-by: Michael Barany <1434605+mbarany@users.noreply.github.com>
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this issue Jul 26, 2022
…ons#625 integrations#620. (integrations#666)

* github_branch_default: send only fields that changed. Fixes integrations#625 integrations#620.

* fix failing test and update docs

Co-authored-by: Jeremy Udit <jcudit@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r/branch_default Type: Bug Something isn't working as documented
Projects
None yet
Development

No branches or pull requests

5 participants