fix(wfctl): fix Spaces bucket bootstrap to use correct DO API endpoint#424
Merged
Conversation
The previous code hit /v2/spaces/{bucket} (check) and /v2/spaces (create),
which don't exist — causing HTTP 404 on every fresh-environment bootstrap.
Changes:
- Check: GET /v2/spaces/buckets/{bucket} (200 → exists, 404 → missing)
- Create: POST /v2/spaces/buckets (201 → created)
- Include server response body in error messages for 4xx/5xx create failures
- Extract bootstrapDOSpacesBucketAt(ctx, bucket, region, apiBase) for testing
Tests: 4 httptest-based cases cover exist-skip, create-new, 4xx-with-body,
and missing-token. GOWORK=off go test ./cmd/wfctl/... -run Bootstrap passes.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes wfctl infra bootstrap’s DigitalOcean Spaces bucket bootstrap logic to use the correct REST API endpoints, and adds tests to validate the expected “exists vs create” behavior and error reporting.
Changes:
- Update DO Spaces bucket check/create URLs to
/v2/spaces/buckets/{bucket}and/v2/spaces/buckets. - Extract
bootstrapDOSpacesBucketAt(..., apiBase)to enable endpoint injection for tests. - Add
httptest-based unit tests and include response body in create error messages.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/wfctl/infra_bootstrap.go | Switches to correct DO Spaces bucket endpoints; factors testable helper; includes response body on create failure. |
| cmd/wfctl/infra_bootstrap_bucket_test.go | Adds tests covering “already exists”, “create new”, “create error includes body”, and “missing token”. |
Comment on lines
122
to
+127
| if resp.StatusCode == http.StatusOK { | ||
| fmt.Printf(" state backend: bucket %q already exists — skipped\n", bucket) | ||
| return nil | ||
| } | ||
|
|
||
| // Create bucket. | ||
| // Create bucket via POST /v2/spaces/buckets. |
There was a problem hiding this comment.
The existence check treats any non-200 response as “bucket missing” and proceeds to create. This will attempt creation even on auth failures (401/403), rate limits (429), or transient server errors (5xx), obscuring the real cause. Consider only creating on 404, and returning a clear error (optionally including response body) for other status codes from the GET check endpoint.
Comment on lines
141
to
+143
| if createResp.StatusCode != http.StatusCreated && createResp.StatusCode != http.StatusOK { | ||
| return fmt.Errorf("create bucket %q: HTTP %d", bucket, createResp.StatusCode) | ||
| respBody, _ := io.ReadAll(createResp.Body) | ||
| return fmt.Errorf("create bucket %q: HTTP %d: %s", bucket, createResp.StatusCode, respBody) |
There was a problem hiding this comment.
On create failures, io.ReadAll(createResp.Body) reads the full response body without bounds and ignores the read error. A large/streaming error response could cause excessive memory usage or hang. Consider reading with a size limit (e.g., io.LimitReader) and handling the read error so the returned message is reliable.
Comment on lines
+28
to
+48
| var postCalled bool | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| switch r.Method { | ||
| case http.MethodGet: | ||
| w.WriteHeader(http.StatusNotFound) | ||
| case http.MethodPost: | ||
| postCalled = true | ||
| w.WriteHeader(http.StatusCreated) | ||
| default: | ||
| http.Error(w, "unexpected: "+r.Method, http.StatusInternalServerError) | ||
| } | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| t.Setenv("DIGITALOCEAN_TOKEN", "test-token") | ||
| if err := bootstrapDOSpacesBucketAt(context.Background(), "my-bucket", "nyc3", srv.URL); err != nil { | ||
| t.Fatalf("expected no error creating new bucket, got: %v", err) | ||
| } | ||
| if !postCalled { | ||
| t.Error("expected POST to be called when bucket does not exist") | ||
| } |
There was a problem hiding this comment.
This test has a data race: postCalled is written by the httptest server handler goroutine and read by the test goroutine. Under go test -race this will fail. Use an atomic, a channel, or synchronize via a mutex/WaitGroup (or assert the POST was called by validating handler behavior and failing the test if it wasn’t).
Comment on lines
+12
to
+18
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method == http.MethodGet { | ||
| w.WriteHeader(http.StatusOK) | ||
| return | ||
| } | ||
| http.Error(w, "unexpected method", http.StatusInternalServerError) | ||
| })) |
There was a problem hiding this comment.
These httptest handlers only branch on HTTP method and don’t assert the request path (e.g., /v2/spaces/buckets/{bucket}) or POST body (name/region). As a result, the tests could pass even if the production code regresses to the wrong endpoint again. Consider validating r.URL.Path (and decoding the JSON body for POST) so the tests actually cover the intended API contract.
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bootstrapDOSpacesBucketto use the correct DO Spaces Buckets REST APIGET /v2/spaces/buckets/{bucket}(was/v2/spaces/{bucket}— doesn't exist → 404)POST /v2/spaces/buckets(was/v2/spaces— doesn't exist → 404)Root Cause
The previous endpoints
/v2/spacesand/v2/spaces/{bucket}don't exist in the DO REST API. The check always returned 404 (interpreted as "bucket doesn't exist"), and the create always returned 404 (reported as an error). The correct path is/v2/spaces/buckets(plural).Design Notes
Authorization: Bearer {DIGITALOCEAN_TOKEN}— no Spaces keys required for bucket management via the REST APIbootstrapDOSpacesBucketAt(ctx, bucket, region, apiBase)for test injection;bootstrapDOSpacesBucketdelegates to it with the production API base URLTests
Four
httptest.NewServer-based tests ininfra_bootstrap_bucket_test.go:TestBootstrapDOSpacesBucket_AlreadyExists— GET 200 → skipTestBootstrapDOSpacesBucket_CreatesNew— GET 404 + POST 201 → createTestBootstrapDOSpacesBucket_CreateErrorIncludesBody— POST 422 → error with response bodyTestBootstrapDOSpacesBucket_MissingToken— clear error when token unsetAll pass:
GOWORK=off go test ./cmd/wfctl/... -run Bootstrap🤖 Generated with Claude Code