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

Fixes accidental deletion of Repository resource from state #1750

Merged

Conversation

t0yv0
Copy link
Contributor

@t0yv0 t0yv0 commented Jun 22, 2023

Resolves #ISSUE_NUMBER


Behavior

Before the change?

Currently when the user does not authenticate the provider, refreshing a GithubRepository resource drops it from the state, which is unexpected and very confusing.

After the change?

The suggested fix consults the resource data to infer the appropriate owner to use instead of the empty owner. For pubic repositories this makes the Read method proceed correctly.

Other information

The suspected root cause of this is: when AnonymousHTTPClient is used, owner == "" and this causes resourceGithubRepositoryRead to issue requests to non-existent URLs such as https://github.com//myrepo and subsequently interpret 404 as a reason to drop the resource from the state.


Additional info

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Added the appropriate label for the given change

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes (Please add the Type: Breaking change label)
  • No

If Yes, what's the impact:

  • N/A

Pull request type

Please add the corresponding label for change this PR introduces:

  • Bugfix: Type: Bug
  • Feature/model/API additions: Type: Feature
  • Updates to docs or samples: Type: Documentation
  • Dependencies/code cleanup: Type: Maintenance

Currently when the user does not authenticate the provider, refreshing a GithubRepository resource drops it from the
state, which is unexpected and very confusing.

The root cause of this is: when AnonymousHTTPClient is used, owner == "" and this causes resourceGithubRepositoryRead to
issue requests to non-existent URLs such as https://github.com//myrepo and subsequently interpret 404 as a reason to
drop the resource from the state.

The suggested fix consults the resource data to infer the appropriate owner to use instead of the empty owner.
Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

In a perfect world, we would have something like this for every resource/data source. Right now using the provider unauthenticated is not a good experience.

Thank you for contributing!

@kfcampbell kfcampbell merged commit 2dfebfe into integrations:main Jun 26, 2023
2 checks passed
kfcampbell added a commit that referenced this pull request Jun 26, 2023
doonga referenced this pull request in doonga/greyrock-ops Jun 29, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [github](https://registry.terraform.io/providers/integrations/github)
([source](https://togithub.com/integrations/terraform-provider-github))
| required_provider | minor | `5.28.1` -> `5.29.0` |

---

### Release Notes

<details>
<summary>integrations/terraform-provider-github (github)</summary>

###
[`v5.29.0`](https://togithub.com/integrations/terraform-provider-github/releases/tag/v5.29.0)

[Compare
Source](https://togithub.com/integrations/terraform-provider-github/compare/v5.28.1...v5.29.0)

#### What's Changed

- fix: support team slug in github_team_membership by
[@&#8203;kristian-lesko](https://togithub.com/kristian-lesko) in
[https://github.com/integrations/terraform-provider-github/pull/1751](https://togithub.com/integrations/terraform-provider-github/pull/1751)
- Fixes accidental deletion of Repository resource from state by
[@&#8203;t0yv0](https://togithub.com/t0yv0) in
[https://github.com/integrations/terraform-provider-github/pull/1750](https://togithub.com/integrations/terraform-provider-github/pull/1750)
- feat: Add GitHub Organization Custom Role Resource and Data Source by
[@&#8203;cailen](https://togithub.com/cailen) in
[https://github.com/integrations/terraform-provider-github/pull/1700](https://togithub.com/integrations/terraform-provider-github/pull/1700)
- Fix typo in deployment branch policy import by
[@&#8203;bpaquet](https://togithub.com/bpaquet) in
[https://github.com/integrations/terraform-provider-github/pull/1758](https://togithub.com/integrations/terraform-provider-github/pull/1758)
- Fix `resourceGithubDependabotOrganizationSecretCreateOrUpdate` by
[@&#8203;frankywahl](https://togithub.com/frankywahl) in
[https://github.com/integrations/terraform-provider-github/pull/1759](https://togithub.com/integrations/terraform-provider-github/pull/1759)
- \[Bug]: Renaming github_repository doesn't taint full_name attribute
by [@&#8203;KenSpur](https://togithub.com/KenSpur) in
[https://github.com/integrations/terraform-provider-github/pull/1756](https://togithub.com/integrations/terraform-provider-github/pull/1756)
- feat: Ability to Manage Codespaces Secrets by
[@&#8203;KenSpur](https://togithub.com/KenSpur) in
[https://github.com/integrations/terraform-provider-github/pull/1729](https://togithub.com/integrations/terraform-provider-github/pull/1729)

#### New Contributors

- [@&#8203;kristian-lesko](https://togithub.com/kristian-lesko) made
their first contribution in
[https://github.com/integrations/terraform-provider-github/pull/1751](https://togithub.com/integrations/terraform-provider-github/pull/1751)
- [@&#8203;t0yv0](https://togithub.com/t0yv0) made their first
contribution in
[https://github.com/integrations/terraform-provider-github/pull/1750](https://togithub.com/integrations/terraform-provider-github/pull/1750)
- [@&#8203;cailen](https://togithub.com/cailen) made their first
contribution in
[https://github.com/integrations/terraform-provider-github/pull/1700](https://togithub.com/integrations/terraform-provider-github/pull/1700)
- [@&#8203;frankywahl](https://togithub.com/frankywahl) made their first
contribution in
[https://github.com/integrations/terraform-provider-github/pull/1759](https://togithub.com/integrations/terraform-provider-github/pull/1759)
- [@&#8203;KenSpur](https://togithub.com/KenSpur) made their first
contribution in
[https://github.com/integrations/terraform-provider-github/pull/1756](https://togithub.com/integrations/terraform-provider-github/pull/1756)

**Full Changelog**:
integrations/terraform-provider-github@v5.28.1...v5.29.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Renovate
Bot](https://togithub.com/renovatebot/renovate).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xNDQuMCIsInVwZGF0ZWRJblZlciI6IjM1LjE0NC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: greyrock-bot <1583719+greyrock-bot[bot]@users.noreply.github.com>
avidspartan1 pushed a commit to avidspartan1/terraform-provider-github that referenced this pull request Feb 5, 2024
…ions#1750)

Currently when the user does not authenticate the provider, refreshing a GithubRepository resource drops it from the
state, which is unexpected and very confusing.

The root cause of this is: when AnonymousHTTPClient is used, owner == "" and this causes resourceGithubRepositoryRead to
issue requests to non-existent URLs such as https://github.com//myrepo and subsequently interpret 404 as a reason to
drop the resource from the state.

The suggested fix consults the resource data to infer the appropriate owner to use instead of the empty owner.

Co-authored-by: Keegan Campbell <[email protected]>
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.

2 participants