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

fix: Use ETag when reading github_branch_default resources. #1994

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

nairb774
Copy link
Contributor

@nairb774 nairb774 commented Nov 2, 2023

This resource was missing saving and using the underlying resource's ETag. This was discovered when we were hitting over-quota errors via this resource.

The etag related implementation was copied from the github_repository implementation.

Aside: There could be some refactoring to have these resources share some common code. Such a refactoring seemed outside the scope of this change.

Resolves #1993


Before the change?

  • Every plan containing a github_branch_default would use one request per resource read.

After the change?

  • github_branch_default should only consume quota when there is drift in the plan.

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
    Note: It didn't look like there were any existing tests validating the etag behaviors. If tests are desired for this feature, a pointer to an example would be appreciated, or a rough sketch if none exists. Any guidance would be appreciated.
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    Note: Not all resources which implement the etag feature have the field documented. The field feels like an internal implementation detail which wouldn't normally be documented. Given the repo is inconsistent, guidance on if the field should be documented or not would be appreciated.

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

This resource was missing saving and using the underlying resource's
ETag. This was discovered when we were hitting over-quota errors via
this resource.

The etag related implementation was copied from the `github_repository`
implementation.

Aside: There could be some refactoring to have these resources share
some common code. Such a refactoring seemed outside the scope of this
change.
@nairb774 nairb774 marked this pull request as ready for review November 2, 2023 00:10
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.

Thanks for doing this!

@kfcampbell kfcampbell merged commit 6b7fd8b into integrations:main Nov 10, 2023
3 checks passed
@kfcampbell
Copy link
Member

PRs to refactor common ETag behavior are of course very welcome should you feel so inclined!

avidspartan1 pushed a commit to avidspartan1/terraform-provider-github that referenced this pull request Feb 5, 2024
…ations#1994)

This resource was missing saving and using the underlying resource's
ETag. This was discovered when we were hitting over-quota errors via
this resource.

The etag related implementation was copied from the `github_repository`
implementation.

Aside: There could be some refactoring to have these resources share
some common code. Such a refactoring seemed outside the scope of this
change.

Co-authored-by: Keegan Campbell <me@kfcampbell.com>
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.

[BUG]: github_branch_default does not use ETags for reads.
2 participants