feat: read timeout for github_ip_ranges data source - #3421
Conversation
Apply a default 5m read timeout and use the Terraform read context for the metadata API call so the timeout is enforced. Reintroduces the intent of the prior stale PR for configurable read timeouts. Refs: integrations#1961 integrations#1978
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
|
Bumping for visibility: this addresses the long-running @KatKorotkikh — same class of pain as in #1961; feel free to weigh in if this matches what you hit in practice. |
| func TestUnitGithubIpRangesDataSource_schemaTimeouts(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| r := dataSourceGithubIpRanges() | ||
| if r.Timeouts == nil { | ||
| t.Fatal("expected Timeouts to be configured on github_ip_ranges data source") | ||
| } | ||
| if r.Timeouts.Read == nil { | ||
| t.Fatal("expected default read timeout to be configured") | ||
| } | ||
| if got := *r.Timeouts.Read; got != 5*time.Minute { | ||
| t.Fatalf("expected default read timeout 5m, got %v", got) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
issue: This test is useless. It only checks that timeout has been configured, it's very brittle. Tests should verify behaviour, and not be tightly coupled to code and it's structure
There was a problem hiding this comment.
Thanks @deiga — fair point, the old test asserted the schema struct rather than any behaviour. Replaced it in c716412 with tests that exercise the read path:
- a mock metadata endpoint that withholds its response, asserting the read aborts at the caller's deadline instead of waiting for the server. This one fails against the previous
owner.StopContextbehaviour, which is the actual bug being fixed. - a normal
/metaresponse, asserting the IP ranges are still populated. - an acceptance sub-test whose config sets
timeouts { read = "2m" }, so the configurable timeout is covered through Terraform rather than by introspecting the schema.
Happy to adjust further if you would rather see this covered purely at the acceptance-test level.
There was a problem hiding this comment.
Pull request overview
These provider review instructions are being used. Adds a configurable five-minute read timeout to prevent stalled GitHub metadata requests.
Changes:
- Configures the data source’s default read timeout.
- Tests the timeout schema.
- Documents timeout configuration.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
github/data_source_github_ip_ranges.go |
Adds the default read timeout. |
github/data_source_github_ip_ranges_test.go |
Verifies timeout configuration. |
templates/data-sources/ip_ranges.md.tmpl |
Documents read timeouts. |
docs/data-sources/ip_ranges.md |
Updates generated documentation. |
Replace the schema introspection assertions with tests that exercise the read path: a stalled metadata endpoint must abort at the deadline instead of waiting for the response, and a normal response must still populate the IP ranges. Also cover a configured timeouts block in the acceptance test.
Before the change?
github_ip_rangesdata source had notimeoutsblock, and the metadata request usedowner.StopContextinstead of the Terraform read context, so a read timeout would not reliably bound the HTTP call.After the change?
github_ip_ranges(overridable viatimeouts { read = "..." }in configuration).ReadContextctx, so Terraform's read deadline is honored.Pull request checklist
Schema migrations: N/A — this updates only the
github_ip_rangesdata source; no managed resource identity or Terraform state schema changes, so no migration code is required.Does this introduce a breaking change?
Please see our docs on breaking changes to help!
Related to #1961
Follow-up to #1978 (previous change went stale; this re-applies the same intent on the current Terraform Plugin SDK v2 codebase.)
/cc @kimxogus @kfcampbell @KatKorotkikh