feat(wfctl): add DigitalOcean App Platform deploy provider#423
Closed
intel352 wants to merge 2 commits into
Closed
feat(wfctl): add DigitalOcean App Platform deploy provider#423intel352 wants to merge 2 commits into
intel352 wants to merge 2 commits into
Conversation
Adds `digitalocean` (alias `do`) to the wfctl deploy provider registry.
- `digitaloceanProvider.Deploy` upserts to the DO App Platform REST API:
GET /v2/apps?name=X to detect existing; POST to create, PUT to update.
Converts ServiceConfig (expose, scaling, secrets) into a minimal doAppSpec.
- `digitaloceanProvider.HealthCheck` fetches live_url from GET /v2/apps/{id}
and delegates to the shared pollHealthCheck helper.
- Auth via DIGITALOCEAN_TOKEN env var; returns a clear error when unset.
- Five tests covering: provider registration, missing token, create-new,
update-existing, and health check — all using httptest.NewServer stubs.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new digitalocean (alias do) deploy provider to wfctl so CI deployments can target DigitalOcean App Platform, including app upsert behavior and post-deploy health checks.
Changes:
- Extend
newDeployProviderto recognizedigitalocean/do. - Implement DigitalOcean App Platform deploy flow (list/find, create, update) and health check live URL resolution.
- Add
httptest-based unit tests covering provider resolution, missing token, create/update deploy, and health check.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| cmd/wfctl/deploy_providers.go | Adds the DigitalOcean deploy provider implementation and wires it into provider selection. |
| cmd/wfctl/deploy_providers_test.go | Adds unit tests for DigitalOcean provider behavior using mock HTTP servers. |
Comment on lines
+748
to
+758
| if err == nil && liveURL != "" { | ||
| hcPath := cfg.Env.HealthCheck.Path | ||
| fullURL := strings.TrimRight(liveURL, "/") + "/" + strings.TrimLeft(hcPath, "/") | ||
| hcCopy := *cfg.Env.HealthCheck | ||
| hcCopy.Path = fullURL | ||
| envCopy := *cfg.Env | ||
| envCopy.HealthCheck = &hcCopy | ||
| cfgCopy := cfg | ||
| cfgCopy.Env = &envCopy | ||
| return pollHealthCheck(ctx, cfgCopy) | ||
| } |
There was a problem hiding this comment.
HealthCheck ignores errors from fetchLiveURL (it only proceeds when err == nil). If the DO API call fails, the code falls back to pollHealthCheck with a relative path, which pollHealthCheck rewrites to http://localhost/... and will almost certainly fail for DigitalOcean deploys. Prefer returning an error (or at least surfacing a warning) when fetchLiveURL fails so the health check doesn’t silently target the wrong host.
Suggested change
| if err == nil && liveURL != "" { | |
| hcPath := cfg.Env.HealthCheck.Path | |
| fullURL := strings.TrimRight(liveURL, "/") + "/" + strings.TrimLeft(hcPath, "/") | |
| hcCopy := *cfg.Env.HealthCheck | |
| hcCopy.Path = fullURL | |
| envCopy := *cfg.Env | |
| envCopy.HealthCheck = &hcCopy | |
| cfgCopy := cfg | |
| cfgCopy.Env = &envCopy | |
| return pollHealthCheck(ctx, cfgCopy) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("fetch DigitalOcean live URL for health check: %w", err) | |
| } | |
| if liveURL == "" { | |
| return fmt.Errorf("fetch DigitalOcean live URL for health check: empty live URL") | |
| } | |
| hcPath := cfg.Env.HealthCheck.Path | |
| fullURL := strings.TrimRight(liveURL, "/") + "/" + strings.TrimLeft(hcPath, "/") | |
| hcCopy := *cfg.Env.HealthCheck | |
| hcCopy.Path = fullURL | |
| envCopy := *cfg.Env | |
| envCopy.HealthCheck = &hcCopy | |
| cfgCopy := cfg | |
| cfgCopy.Env = &envCopy | |
| return pollHealthCheck(ctx, cfgCopy) |
Comment on lines
+418
to
+431
| var postBody []byte | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| switch { | ||
| case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/v2/apps"): | ||
| w.Header().Set("Content-Type", "application/json") | ||
| json.NewEncoder(w).Encode(doListAppsResponse{Apps: []doApp{}}) | ||
| case r.Method == http.MethodPost && r.URL.Path == "/v2/apps": | ||
| postBody, _ = io.ReadAll(r.Body) | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusCreated) | ||
| json.NewEncoder(w).Encode(doAppResponse{App: doApp{ID: "new-app-1"}}) | ||
| default: | ||
| http.Error(w, "unexpected: "+r.Method+" "+r.URL.Path, http.StatusInternalServerError) | ||
| } |
There was a problem hiding this comment.
This test captures postBody from the httptest handler goroutine and then reads it from the test goroutine without synchronization. With go test -race this will be flagged as a data race. Use a channel, sync.Mutex, or atomic.Value to safely pass the captured body back to the test.
Comment on lines
+454
to
+468
| var putPath string | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| switch { | ||
| case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/v2/apps"): | ||
| w.Header().Set("Content-Type", "application/json") | ||
| json.NewEncoder(w).Encode(doListAppsResponse{Apps: []doApp{ | ||
| {ID: "existing-1", Spec: doAppSpec{Name: "myapp"}}, | ||
| }}) | ||
| case r.Method == http.MethodPut: | ||
| putPath = r.URL.Path | ||
| w.Header().Set("Content-Type", "application/json") | ||
| json.NewEncoder(w).Encode(doAppResponse{App: doApp{ID: "existing-1"}}) | ||
| default: | ||
| http.Error(w, "unexpected: "+r.Method+" "+r.URL.Path, http.StatusInternalServerError) | ||
| } |
There was a problem hiding this comment.
This test captures putPath inside the httptest handler goroutine and reads it in the test goroutine without synchronization. This is a data race under go test -race. Use a channel / mutex / atomic to transfer the observed request path safely.
Comment on lines
+638
to
+640
| func (p *digitaloceanProvider) findApp(ctx context.Context, token, name string) (string, error) { | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, p.doBase()+"/v2/apps?name="+name, nil) | ||
| if err != nil { |
There was a problem hiding this comment.
findApp builds the request URL by concatenating "?name="+name without URL-escaping. App names containing spaces or reserved characters will produce an invalid request (and it also allows query injection). Build the URL using net/url (url.Values / QueryEscape) instead of string concatenation.
Comment on lines
+585
to
+602
| instanceCount := 1 | ||
| if len(cfg.Services) == 1 { | ||
| for _, svc := range cfg.Services { | ||
| if svc.Scaling != nil && svc.Scaling.Replicas > 0 { | ||
| instanceCount = svc.Scaling.Replicas | ||
| } | ||
| } | ||
| } | ||
|
|
||
| httpPort := 8080 | ||
| if len(cfg.Services) == 1 { | ||
| for _, svc := range cfg.Services { | ||
| if len(svc.Expose) > 0 { | ||
| httpPort = svc.Expose[0].Port | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
buildAppSpec silently falls back to a single service spec (using defaults for port/replicas) when len(cfg.Services) > 1. In multi-service CI configs this will deploy an incomplete/incorrect app without any warning. Either implement multi-service mapping for DigitalOcean App Platform or return a clear error when multiple services are provided.
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…w fixes) - Add injectable *http.Client field to digitaloceanProvider with 2-min default - Replace all http.DefaultClient.Do calls with p.httpClient().Do - Use url.QueryEscape for app name in GET /v2/apps query param - Warn when fetchLiveURL fails in HealthCheck instead of silently falling back - Log warning when deploying multiple services via DO App Platform Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
Pivoting: Task 5 reimplemented DO App Platform logic that already exists in workflow-plugin-digitalocean's AppPlatformDriver. The correct architectural path is to make wfctl ci run --phase deploy plugin-aware so workflow-plugin-digitalocean's IaCProvider + DeployDriverProvider extension is the source of truth. Closing this PR; follow-up task will do the plugin-delegation refactor. |
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
digitalocean(and aliasdo) tonewDeployProviderindeploy_providers.goDeployupserts to DO App Platform REST API: GET to find existing app, POST to create, PUT to update. TranslatesServiceConfig(expose ports, scaling replicas, secrets) to a minimaldoAppSpecHealthCheckfetcheslive_urlfrom GET/v2/apps/{id}and passes the full URL topollHealthCheckDIGITALOCEAN_TOKENenv var; clear error when unsetgodois already ingo.modTests
Five
httptest.NewServer-based tests indeploy_providers_test.go:TestDigitalOceanProvider_NewProvider— alias "do" and "digitalocean" both resolveTestDigitalOceanProvider_MissingToken— clear error when token unsetTestDigitalOceanProvider_Deploy_CreatesNewApp— POST /v2/apps called with app nameTestDigitalOceanProvider_Deploy_UpdatesExistingApp— PUT /v2/apps/{id} calledTestDigitalOceanProvider_HealthCheck— live_url fetched and polledAll pass:
GOWORK=off go test ./cmd/wfctl/... -run DigitalOceanContext
BMW's
infra.yamldeclaresprovider: digitalocean; wfctl was erroring with"unsupported deploy provider". This unblocks BMW deploys.🤖 Generated with Claude Code