Adding timeouts for resource create/update#278
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds configurable per-operation timeouts to supabase_project and supabase_settings. Introduces an optional Sequence Diagram(s)sequenceDiagram
participant Terraform
participant Provider
participant TimeoutsPkg
participant APIClient
participant WaitUtils
Terraform->>Provider: apply resource create
Provider->>TimeoutsPkg: read data.Timeouts.Create
TimeoutsPkg-->>Provider: returns createTimeout
Provider->>Provider: ctx, cancel := context.WithTimeout(createTimeout)
Provider->>APIClient: createProject(ctx, params, timeout=createTimeout)
APIClient-->>Provider: project created (id)
Provider->>WaitUtils: waitForProjectActive(ctx, projectID, client, timeout=createTimeout)
WaitUtils->>APIClient: poll project status (with timeout)
APIClient-->>WaitUtils: status=active
WaitUtils-->>Provider: ready
Provider->>Terraform: set state / finish create
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/resources/project.md (1)
1-44:⚠️ Potential issue | 🟡 MinorRegenerate documentation instead of manual edits.
The pipeline detects a code generation diff. This file is auto-generated by
terraform-plugin-docs. Rungo generate ./...or the appropriate docs generation command to regenerate from the schema, rather than editing manually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/resources/project.md` around lines 1 - 44, The generated docs file for the supabase_project resource was edited manually instead of regenerating; revert manual edits and regenerate the documentation from the schema using the docs generator (terraform-plugin-docs) — e.g., run the project’s doc generation command (go generate ./... or the repo’s documented generation step) so the supabase_project resource documentation and its timeouts block are re-created from the source schema rather than edited by hand.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/provider/settings_resource_test.go`:
- Around line 523-693: The TestAccSettingsResource_Timeouts test function has
gofmt/formatting issues (mismatch with the style used in settings_resource.go);
run go fmt ./... (or gofmt -w .) to reformat the file containing
TestAccSettingsResource_Timeouts, verify the function signature and the request
mock blocks match the formatting style used in settings_resource.go, then stage
and commit the formatted changes.
In `@internal/provider/settings_resource.go`:
- Around line 15-16: Formatting of internal/provider/settings_resource.go is
incorrect; run gofmt to fix it and ensure imports and file formatting comply
with gofmt. Run `gofmt -w internal/provider/settings_resource.go` (or `go fmt
./...`) to automatically reformat the file, then verify the import block
including
"github.com/hashicorp/terraform-plugin-framework-timeouts/resource/timeouts" and
"github.com/hashicorp/terraform-plugin-framework-jsontypes/jsontypes" is
ordered/organized per gofmt rules and commit the formatted file.
---
Outside diff comments:
In `@docs/resources/project.md`:
- Around line 1-44: The generated docs file for the supabase_project resource
was edited manually instead of regenerating; revert manual edits and regenerate
the documentation from the schema using the docs generator
(terraform-plugin-docs) — e.g., run the project’s doc generation command (go
generate ./... or the repo’s documented generation step) so the supabase_project
resource documentation and its timeouts block are re-created from the source
schema rather than edited by hand.
---
Duplicate comments:
In `@docs/resources/settings.md`:
- Around line 1-76: The settings documentation has been manually edited instead
of being generated; revert the manual changes for the supabase_settings resource
and re-run the documentation generator (terraform-plugin-docs / tfplugindocs) to
produce an authoritative docs/resources output for the supabase_settings
resource, then commit the regenerated file; ensure the generated schema, example
usage, and timeouts block come from the generator rather than hand edits.
There was a problem hiding this comment.
Pull request overview
Adds configurable Terraform timeouts to avoid long-running Supabase resource operations timing out, by introducing a timeouts block and plumbing the configured duration into the provider’s polling/wait logic.
Changes:
- Added
timeoutsblocks tosupabase_projectandsupabase_settingsresources and enforced them viacontext.WithTimeoutin Create/Update/Delete. - Updated
waitForProjectActive/waitForServicesActivehelpers to accept a caller-provided timeout duration. - Added acceptance tests and documentation updates for the new timeouts configuration; introduced
terraform-plugin-framework-timeoutsdependency.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/provider/utils.go | Makes project/service wait helpers accept an explicit timeout instead of using a hardcoded default. |
| internal/provider/utils_test.go | Updates unit tests to pass an explicit timeout duration to the updated wait helpers. |
| internal/provider/settings_resource.go | Adds timeouts block support and applies configured timeouts to Create/Update execution. |
| internal/provider/settings_resource_test.go | Adds an acceptance test ensuring timeouts block is accepted for settings resource. |
| internal/provider/project_resource.go | Adds timeouts block support and applies configured timeouts to Create/Update/Delete execution and provisioning waits. |
| internal/provider/project_resource_test.go | Adds an acceptance test ensuring timeouts block is accepted for project resource. |
| docs/resources/settings.md | Documents timeouts block for settings resource and adds it to example usage. |
| docs/resources/project.md | Documents timeouts block for project resource and adds it to example usage. |
| go.mod | Adds terraform-plugin-framework-timeouts dependency. |
| go.sum | Adds checksums for terraform-plugin-framework-timeouts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/provider/settings_resource_test.go`:
- Around line 514-520: The test inserts a timeouts block including `delete =
"30m"`, but the resource schema in settings_resource.go currently has Delete:
false, so Terraform will reject that argument; fix the test in
settings_resource_test.go to remove the `delete = "30m"` line from the injected
timeouts block (leave only create and update), or alternatively enable Delete by
changing the schema's Delete to true on the resource in
settings_resource.go—pick the approach consistent with intended behavior (prefer
removing the unsupported `delete` in the test if delete should remain
unsupported).
- Revert manual edits to docs/resources/project.md and docs/resources/settings.md - Regenerate using terraform-plugin-docs with provider schema export - Schema includes timeouts block; generated docs show timeouts from schema - docs/schema.json updated with current provider schema and supabase key for generator Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rncain
changed the title
|
Hey @rncain, thanks for the contribution! Did you run into a timeout with the current defaults? If so, was it while provisioning a project resource? |
Ya, it was while creating a new project. I don't seem to have the original error message. But once I added this config block, it was working well 👍 |
Gotcha! I was mainly curious because I'm trying to understand the best way to tweak the defaults based on your experience, but no need to dig up the original error - just wanted to share why I asked 🙂 |
savme
left a comment
savme
left a comment
There was a problem hiding this comment.
This looks good! I left a couple of comments below.
Also, could you rebase onto main? The CI issue is unrelated to your change and has already been fixed in main. A rebase should get the checks passing
| ctx, cancel := context.WithTimeout(ctx, createTimeout) | ||
| defer cancel() |
There was a problem hiding this comment.
| ctx, cancel := context.WithTimeout(ctx, createTimeout) | |
| defer cancel() |
I think handling timeouts exclusively in waitForProjectActive would simplify things a bit.
With both context and StateChangeConf in play, whichever fires first wins, making the timeout diagnostic inconsistent. I think there's also a subtle risk that a deadline-bound context gets reused downstream (e.g. resp.State.Set) with most of its time budget already consumed.
| return | ||
| } | ||
|
|
||
| deleteTimeout, diags := data.Timeouts.Delete(ctx, defaultWaitTimeout) |
There was a problem hiding this comment.
Let's keep the delete timeout fixed, since there's no polling involved
What kind of change does this PR introduce?
feature
What is the current behavior?
Project/settings creation can timeout
What is the new behavior?
Add config changes that will allow timeout adjustments to ensure resource CRUD doesn't time out.
Additional context