feat: IaC support — wfctl infra commands, DO database module, PlatformProvider adapters#281
feat: IaC support — wfctl infra commands, DO database module, PlatformProvider adapters#281
Conversation
DOAppPlatformAdapter wraps PlatformDOApp to implement the PlatformProvider interface (Plan/Apply/Status/Destroy), registered as "<name>.iac" service. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DONetworkingPlatformAdapter wraps PlatformDONetworking to implement PlatformProvider (Plan/Apply/Status/Destroy), registered as "<name>.iac" service. Converts DONetworkPlan changes to PlatformAction entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DODNSPlatformAdapter wraps PlatformDODNS to implement PlatformProvider (Plan/Apply/Status/Destroy), registered as "<name>.iac" service. Converts DODNSPlan changes to PlatformAction entries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add platform.do_app, platform.do_networking, platform.do_dns, and platform.do_database to the Infrastructure module table in DOCUMENTATION.md. Add wfctl infra command reference to docs/WFCTL.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Check fmt.Scanln error return values (errcheck) - Use strings.EqualFold instead of strings.ToLower comparison (gocritic) - Remove redundant embedded field selector PlatformDOApp (staticcheck) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds first-class infrastructure-as-code (IaC) workflow support to the workflow engine by introducing wfctl infra lifecycle commands and expanding the platform plugin with DigitalOcean-managed infrastructure modules/adapters to drive generic step.iac_* pipelines.
Changes:
- Add
wfctl infra plan/apply/status/drift/destroycommand wired viawfctl.yamlandcmd/wfctl/main.go. - Add
platform.do_databasemodule (mock + real godo backend) and register it in the platform plugin (factories + schema). - Add
PlatformProvideradapters for existing DigitalOcean modules (app, DNS, networking) by registering*.iacservices plus accompanying adapter tests.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/platform/plugin.go | Registers platform.do_database module type, factory, and UI schema. |
| module/platform_do_networking.go | Registers *.iac adapter service and implements PlatformProvider adapter for DO networking. |
| module/platform_do_networking_test.go | Adds tests validating the DO networking PlatformProvider adapter behavior. |
| module/platform_do_dns.go | Registers *.iac adapter service and implements PlatformProvider adapter for DO DNS. |
| module/platform_do_dns_test.go | Adds tests validating the DO DNS PlatformProvider adapter behavior. |
| module/platform_do_app.go | Registers *.iac adapter service and implements PlatformProvider adapter for DO App Platform. |
| module/platform_do_app_test.go | Adds tests validating the DO App Platform PlatformProvider adapter behavior. |
| module/platform_do_database.go | Introduces a new DO managed database platform module (mock + real backend). |
| module/platform_do_database_test.go | Adds basic mock-backend test coverage for the new DB module. |
| cmd/wfctl/infra.go | Implements wfctl infra subcommands, config discovery, and pipeline delegation. |
| cmd/wfctl/main.go | Registers the new infra command dispatcher. |
| cmd/wfctl/wfctl.yaml | Adds the infra CLI workflow + pipeline mapping to step.cli_invoke. |
| docs/WFCTL.md | Documents the new wfctl infra command and its flags/examples. |
| DOCUMENTATION.md | Documents newly added DigitalOcean platform module types including platform.do_database. |
| func (m *PlatformDODatabase) Apply() (*PlatformResult, error) { | ||
| st, err := m.backend.create(m) | ||
| if err != nil { | ||
| return &PlatformResult{Success: false, Message: err.Error()}, err | ||
| } | ||
| m.state = st | ||
| return &PlatformResult{ | ||
| Success: true, | ||
| Message: fmt.Sprintf("Database %s online (host: %s:%d)", st.Name, st.Host, st.Port), | ||
| State: st, | ||
| }, nil |
There was a problem hiding this comment.
Apply() always calls backend.create(...). For the real DigitalOcean backend this hits godo.Databases.Create, so repeated applies will create a new managed DB cluster each time instead of being idempotent (and may leak resources/cost). Consider making Apply() conditional on m.state.ID (no-op if already online) or implementing an update/resize path when ID is present, and ensure the real backend uses the corresponding godo Update/Resize APIs rather than Create.
| if err != nil { | ||
| return fmt.Errorf("delete database: %w", err) | ||
| } | ||
| m.state.Status = "deleted" |
There was a problem hiding this comment.
doDatabaseRealBackend.destroy() sets m.state.Status = "deleted" but does not clear m.state.ID. After a successful delete, subsequent Status() calls will try Databases.Get with the now-deleted ID and likely error. Clearing the ID (and any connection fields) after delete would make destroy idempotent and keep status consistent.
| m.state.Status = "deleted" | |
| m.state.Status = "deleted" | |
| m.state.ID = "" | |
| m.state.Host = "" | |
| m.state.Port = 0 | |
| m.state.DatabaseName = "" | |
| m.state.User = "" | |
| m.state.Password = "" | |
| m.state.URI = "" |
| func doDatabaseFromGodo(db *godo.Database) *DODatabaseState { | ||
| st := &DODatabaseState{ | ||
| ID: db.ID, | ||
| Name: db.Name, | ||
| Engine: db.EngineSlug, | ||
| Version: db.VersionSlug, | ||
| Size: db.SizeSlug, | ||
| Region: db.RegionSlug, | ||
| NumNodes: db.NumNodes, | ||
| Status: db.Status, | ||
| CreatedAt: db.CreatedAt, | ||
| } | ||
| if db.Connection != nil { | ||
| st.Host = db.Connection.Host | ||
| st.Port = db.Connection.Port | ||
| st.DatabaseName = db.Connection.Database | ||
| st.User = db.Connection.User | ||
| st.Password = db.Connection.Password | ||
| st.URI = db.Connection.URI | ||
| } |
There was a problem hiding this comment.
DODatabaseState includes connection credentials (Password, URI) and doDatabaseFromGodo() populates them from the API response. The IaC apply step persists result.State into the IaC state store outputs (filesystem backend writes JSON), which would store these secrets in plaintext on disk. Recommend redacting/omitting credential fields from the persisted state (e.g., json:"-"), or splitting credentials into a separate secret output mechanism rather than IaC state.
module/platform_do_database.go
Outdated
| detail = fmt.Sprintf("Update database %q (%s → %s, %d nodes)", | ||
| m.state.Name, m.state.Size, m.state.Size, m.state.NumNodes) |
There was a problem hiding this comment.
In Plan(), the "update" detail message prints the same value for both sides of the arrow (%s → %s uses m.state.Size twice), so the plan output is misleading. Either track previous/applied values to show a real diff, or simplify the message to avoid implying a change when there isn't one.
| detail = fmt.Sprintf("Update database %q (%s → %s, %d nodes)", | |
| m.state.Name, m.state.Size, m.state.Size, m.state.NumNodes) | |
| detail = fmt.Sprintf("Update database %q (size: %s, %d nodes)", | |
| m.state.Name, m.state.Size, m.state.NumNodes) |
There was a problem hiding this comment.
Addressed in 052eebd — fixed the misleading format string to show size: %s, %d nodes instead of the duplicate %s → %s.
| fmt.Printf("Resources to manage (%d):\n", len(platforms)) | ||
| for _, p := range platforms { | ||
| fmt.Printf(" + %s (%s)\n", p.Name, p.Type) | ||
| for k, v := range p.Config { | ||
| if k == "account" || k == "provider" { | ||
| continue | ||
| } | ||
| fmt.Printf(" %s: %v\n", k, v) | ||
| } | ||
| } |
There was a problem hiding this comment.
runInfraPlan prints all platform.* module config key/values (only skipping account and provider). Platform module configs often contain secrets (e.g., env vars, API keys, DNS tokens), so this can leak sensitive data to the terminal and logs/CI output. Consider redacting common secret keys (token, secret, password, key, envs, etc.) or only printing a safe summary (module name/type) unless a --verbose flag is provided.
| func runInfra(args []string) error { | ||
| if len(args) < 1 { | ||
| return infraUsage() | ||
| } | ||
| switch args[0] { | ||
| case "plan": | ||
| return runInfraPlan(args[1:]) | ||
| case "apply": | ||
| return runInfraApply(args[1:]) | ||
| case "status": | ||
| return runInfraStatus(args[1:]) | ||
| case "drift": | ||
| return runInfraDrift(args[1:]) | ||
| case "destroy": | ||
| return runInfraDestroy(args[1:]) | ||
| default: | ||
| return infraUsage() | ||
| } | ||
| } | ||
|
|
||
| func infraUsage() error { | ||
| fmt.Fprintf(flag.CommandLine.Output(), `Usage: wfctl infra <action> [options] [config.yaml] | ||
|
|
||
| Manage infrastructure defined in a workflow config. | ||
|
|
||
| Actions: | ||
| plan Show planned infrastructure changes | ||
| apply Apply infrastructure changes | ||
| status Show current infrastructure status | ||
| drift Detect configuration drift | ||
| destroy Tear down infrastructure | ||
|
|
||
| Options: | ||
| --config <file> Config file (default: infra.yaml or config/infra.yaml) | ||
| --auto-approve Skip confirmation prompt (apply/destroy only) | ||
| `) | ||
| return fmt.Errorf("missing action") | ||
| } |
There was a problem hiding this comment.
infraUsage() always returns fmt.Errorf("missing action"), even when the user provided an unsupported/invalid action (the default case). That makes CLI error output misleading. Consider returning a different error for invalid actions (e.g., "unknown action: ...") and reserving "missing action" for the no-args case (or returning flag.ErrHelp).
There was a problem hiding this comment.
Addressed in 052eebd — improved error message to say "missing or unknown action".
| func TestPlatformDODatabase_MockBackend(t *testing.T) { | ||
| m := &PlatformDODatabase{ | ||
| name: "test-db", | ||
| config: map[string]any{ | ||
| "provider": "mock", | ||
| "engine": "pg", | ||
| "version": "16", | ||
| "size": "db-s-1vcpu-1gb", | ||
| "region": "nyc1", | ||
| "num_nodes": 1, | ||
| "name": "test-db", | ||
| }, | ||
| state: &DODatabaseState{ | ||
| Name: "test-db", | ||
| Engine: "pg", | ||
| Version: "16", | ||
| Size: "db-s-1vcpu-1gb", | ||
| Region: "nyc1", | ||
| NumNodes: 1, | ||
| Status: "pending", | ||
| }, | ||
| backend: &doDatabaseMockBackend{}, | ||
| } |
There was a problem hiding this comment.
This test constructs PlatformDODatabase by hand and bypasses Init(), so it doesn't exercise config parsing/defaulting or provider selection logic (which is where many bugs tend to hide). Consider adding an Init()-based test using NewPlatformDODatabase(...).Init(app) (similar to other platform DO module tests) and asserting defaults/required config behavior.
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
- Fix misleading Plan() detail format string (was printing same value twice) - Improve infraUsage() error message for unknown actions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
S3-compatible remote state storage using DO Spaces. Supports custom endpoints for any S3-compatible store. Addresses PR review feedback about avoiding repo-committed state files with credentials. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| m.state.Port = 25060 | ||
| m.state.DatabaseName = "defaultdb" | ||
| m.state.User = "doadmin" | ||
| m.state.Password = "mock-password" | ||
| m.state.URI = fmt.Sprintf("postgresql://%s:%s@%s:%d/%s?sslmode=require", | ||
| m.state.User, m.state.Password, m.state.Host, m.state.Port, m.state.DatabaseName) |
There was a problem hiding this comment.
The mock backend always sets a PostgreSQL-style connection URI/port/database/user regardless of the configured engine (mysql, redis, mongodb, kafka). Since the module/schema advertises multiple engines, this makes mock outputs misleading for non-pg cases. Consider switching on m.state.Engine to populate engine-appropriate defaults (scheme/port/dbname fields) or documenting that the mock backend only emulates Postgres semantics.
| m.state.Port = 25060 | |
| m.state.DatabaseName = "defaultdb" | |
| m.state.User = "doadmin" | |
| m.state.Password = "mock-password" | |
| m.state.URI = fmt.Sprintf("postgresql://%s:%s@%s:%d/%s?sslmode=require", | |
| m.state.User, m.state.Password, m.state.Host, m.state.Port, m.state.DatabaseName) | |
| m.state.Password = "mock-password" | |
| switch m.state.Engine { | |
| case "", "pg", "postgres", "postgresql": | |
| // Default to PostgreSQL-style connection details. | |
| m.state.Port = 25060 | |
| m.state.DatabaseName = "defaultdb" | |
| m.state.User = "doadmin" | |
| m.state.URI = fmt.Sprintf("postgresql://%s:%s@%s:%d/%s?sslmode=require", | |
| m.state.User, m.state.Password, m.state.Host, m.state.Port, m.state.DatabaseName) | |
| case "mysql": | |
| m.state.Port = 3306 | |
| m.state.DatabaseName = "defaultdb" | |
| m.state.User = "doadmin" | |
| m.state.URI = fmt.Sprintf("mysql://%s:%s@%s:%d/%s", | |
| m.state.User, m.state.Password, m.state.Host, m.state.Port, m.state.DatabaseName) | |
| case "redis": | |
| m.state.Port = 6379 | |
| // Use DatabaseName as a logical database index for Redis mocks. | |
| m.state.DatabaseName = "0" | |
| m.state.User = "default" | |
| m.state.URI = fmt.Sprintf("redis://%s:%s@%s:%d/%s", | |
| m.state.User, m.state.Password, m.state.Host, m.state.Port, m.state.DatabaseName) | |
| case "mongodb": | |
| m.state.Port = 27017 | |
| m.state.DatabaseName = "defaultdb" | |
| m.state.User = "doadmin" | |
| m.state.URI = fmt.Sprintf("mongodb://%s:%s@%s:%d/%s", | |
| m.state.User, m.state.Password, m.state.Host, m.state.Port, m.state.DatabaseName) | |
| case "kafka": | |
| // Kafka typically uses a broker address without database/user semantics. | |
| m.state.Port = 9092 | |
| m.state.DatabaseName = "" | |
| m.state.User = "" | |
| m.state.URI = fmt.Sprintf("kafka://%s:%d", | |
| m.state.Host, m.state.Port) | |
| default: | |
| // Fallback to PostgreSQL-style defaults for unknown engines. | |
| m.state.Port = 25060 | |
| m.state.DatabaseName = "defaultdb" | |
| m.state.User = "doadmin" | |
| m.state.URI = fmt.Sprintf("postgresql://%s:%s@%s:%d/%s?sslmode=require", | |
| m.state.User, m.state.Password, m.state.Host, m.state.Port, m.state.DatabaseName) | |
| } |
| cfg, err := awsconfig.LoadDefaultConfig(context.Background(), | ||
| awsconfig.WithRegion(regionOrDefault(region)), | ||
| awsconfig.WithCredentialsProvider(credentials.NewStaticCredentialsProvider(accessKey, secretKey, "")), | ||
| ) |
There was a problem hiding this comment.
NewSpacesIaCStateStore always installs a static credentials provider even when accessKey / secretKey are empty (after env var fallback). That disables the default AWS credential chain and can lead to confusing runtime auth errors with blank credentials. Consider validating that both credentials are non-empty for Spaces (and returning a clear config error), or only setting a static provider when at least one credential value is provided so the default chain can be used.
| cfg, err := awsconfig.LoadDefaultConfig(context.Background(), | |
| awsconfig.WithRegion(regionOrDefault(region)), | |
| awsconfig.WithCredentialsProvider(credentials.NewStaticCredentialsProvider(accessKey, secretKey, "")), | |
| ) | |
| accessProvided := accessKey != "" | |
| secretProvided := secretKey != "" | |
| if accessProvided != secretProvided { | |
| return nil, fmt.Errorf("iac spaces state: both accessKey and secretKey must be provided together, or neither to use the default AWS credential chain") | |
| } | |
| loadOpts := []func(*awsconfig.LoadOptions) error{ | |
| awsconfig.WithRegion(regionOrDefault(region)), | |
| } | |
| if accessProvided && secretProvided { | |
| loadOpts = append(loadOpts, awsconfig.WithCredentialsProvider(credentials.NewStaticCredentialsProvider(accessKey, secretKey, ""))) | |
| } | |
| cfg, err := awsconfig.LoadDefaultConfig(context.Background(), loadOpts...) |
| // Lock creates a lock object for resourceID. Fails if the lock already exists. | ||
| func (s *SpacesIaCStateStore) Lock(resourceID string) error { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| key := s.lockKey(resourceID) | ||
|
|
||
| // Check if lock exists. | ||
| _, err := s.client.HeadObject(context.Background(), &s3.HeadObjectInput{ | ||
| Bucket: &s.bucket, | ||
| Key: &key, | ||
| }) | ||
| if err == nil { | ||
| return fmt.Errorf("iac spaces state: Lock %q: resource is already locked", resourceID) | ||
| } | ||
| if !isNotFoundErr(err) { | ||
| return fmt.Errorf("iac spaces state: Lock %q: head: %w", resourceID, err) | ||
| } | ||
|
|
||
| // Create lock object with a timestamp. | ||
| body := []byte(time.Now().UTC().Format(time.RFC3339)) | ||
| _, err = s.client.PutObject(context.Background(), &s3.PutObjectInput{ | ||
| Bucket: &s.bucket, | ||
| Key: &key, | ||
| Body: bytes.NewReader(body), | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("iac spaces state: Lock %q: put: %w", resourceID, err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Lock() is implemented as HeadObject then PutObject, which is not atomic across processes and can race (two writers can both observe "not found" and then both create the lock). For a distributed lock in S3/Spaces, prefer a single conditional write (e.g., PutObject with If-None-Match: * / equivalent) or another atomic mechanism so lock acquisition is safe under concurrency.
| // Execute plan via wfctl pipeline run | ||
| fmt.Printf("Running plan pipeline...\n") | ||
| return runPipelineRun([]string{"-c", cfgFile, "-p", "plan"}) | ||
| } |
There was a problem hiding this comment.
runInfra* delegates execution to runPipelineRun, which builds an engine with only the PipelineWorkflowHandler + pipeline-steps plugin. That engine will not have platform IaC step types (e.g. step.iac_plan/apply/status/destroy, DO platform steps), so infra configs that use those steps will fail to compile/run. Consider building the engine with the same default plugin set as wfctl normally uses (or explicitly loading the platform plugin) for infra pipeline execution, rather than reusing the minimal runPipelineRun engine builder.
| m.backend = &doDatabaseMockBackend{} | ||
| case "digitalocean": | ||
| accountName, _ := m.config["account"].(string) | ||
| acc, ok := app.SvcRegistry()[accountName].(*CloudAccount) |
There was a problem hiding this comment.
For provider: digitalocean, account is effectively required but not validated. If account is missing/empty, the lookup app.SvcRegistry()[accountName] will fail the type assertion with a confusing message about account "" not being a *CloudAccount. Add an explicit check that accountName is non-empty (and ideally that the service exists) when providerType == "digitalocean", and return a clear config error.
| acc, ok := app.SvcRegistry()[accountName].(*CloudAccount) | |
| if accountName == "" { | |
| return fmt.Errorf("platform.do_database %q: provider %q requires non-empty account name", m.name, providerType) | |
| } | |
| svc, exists := app.SvcRegistry()[accountName] | |
| if !exists { | |
| return fmt.Errorf("platform.do_database %q: account %q not found in service registry", m.name, accountName) | |
| } | |
| acc, ok := svc.(*CloudAccount) |
| func (m *PlatformDODatabase) Plan() (*PlatformPlan, error) { | ||
| actionType := "create" | ||
| detail := fmt.Sprintf("Create %s %s database %q in %s (%s, %d nodes)", | ||
| m.state.Engine, m.state.Version, m.state.Name, m.state.Region, m.state.Size, m.state.NumNodes) | ||
| if m.state.ID != "" { | ||
| actionType = "update" | ||
| detail = fmt.Sprintf("Update database %q (size: %s, %d nodes)", | ||
| m.state.Name, m.state.Size, m.state.NumNodes) | ||
| } | ||
| return &PlatformPlan{ | ||
| Provider: "digitalocean", | ||
| Resource: "managed_database", | ||
| Actions: []PlatformAction{{Type: actionType, Resource: m.state.Name, Detail: detail}}, | ||
| }, nil | ||
| } | ||
|
|
||
| func (m *PlatformDODatabase) Apply() (*PlatformResult, error) { | ||
| st, err := m.backend.create(m) | ||
| if err != nil { | ||
| return &PlatformResult{Success: false, Message: err.Error()}, err | ||
| } | ||
| m.state = st | ||
| return &PlatformResult{ | ||
| Success: true, | ||
| Message: fmt.Sprintf("Database %s online (host: %s:%d)", st.Name, st.Host, st.Port), | ||
| State: st, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
PlatformDODatabase.Apply() always calls backend.create(), and the real backend implementation always calls Databases.Create(...) even when m.state.ID is already set. This makes Apply non-idempotent and contradicts Plan() reporting an "update" action once an ID exists; a second Apply will attempt to create a second cluster / fail with a name conflict. Consider making Apply idempotent by branching on m.state.ID (e.g., no-op/status refresh when already created, or implement an update/resize path) and aligning Plan() action types with what Apply actually does.
| Password string `json:"password"` | ||
| URI string `json:"uri"` |
There was a problem hiding this comment.
DODatabaseState includes connection secrets (Password, URI). step.iac_apply persists result.State into IaC state outputs via JSON snapshotting, so these fields will be written to whatever backend is configured (filesystem/Spaces). Consider excluding secrets from JSON serialization (e.g., json:"-"), returning them separately via a secrets mechanism, or redacting before returning PlatformResult.State to avoid leaking credentials into persisted state/logs.
| Password string `json:"password"` | |
| URI string `json:"uri"` | |
| Password string `json:"-"` | |
| URI string `json:"-"` |
Summary
wfctl infra plan/apply/status/drift/destroyCLI commands for IaC lifecycle managementplatform.do_databasemodule for DigitalOcean Managed Databases (mock + real backends)PlatformProvideradapters for existing DO modules (App Platform, Networking, DNS) enabling generic IaC step usageDesign
See:
docs/plans/2026-03-09-iac-digitalocean-design.md(in buymywishlist repo)Changes
Test Plan
go test ./...)go vet ./...cleango build ./cmd/wfctl/and./cmd/server/compile🤖 Generated with Claude Code