refactor: Phase B core deletion — drop in-core spaces/s3 + rewrite AWS resolvers SDK-free#687
Merged
Merged
Conversation
…rker + warn, no SDK
…in-served via workflow-plugin-digitalocean v1.1.0
…ive via workflow-plugin-aws v1.1.0
Contributor
There was a problem hiding this comment.
Pull request overview
This PR completes “Plan-2 Phase B” core deletions by removing in-core S3/Spaces storage + state implementations and making AWS credential resolvers SDK-free, shifting those responsibilities to provider plugins (workflow-plugin-aws / workflow-plugin-digitalocean).
Changes:
- Removed in-core
storage.s3module andstep.s3_uploadpipeline step (plus core plugin registrations/tests/docs). - Removed in-core DigitalOcean Spaces IaC state backend (and wfctl direct-path Spaces backend support), relying on plugin-served backends instead.
- Reworked AWS
profile/role_arncredential resolvers to “declare, don’t resolve” viacredential_sourcemarkers, adding focused unit tests and a Phase B migration doc.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/storage/plugin.go | Drops storage.s3 from manifest, factories, and module schemas. |
| plugins/storage/plugin_test.go | Updates manifest/schema/factory expectations after storage.s3 removal. |
| plugins/pipelinesteps/plugin.go | Removes step.s3_upload from manifest and step factory registrations. |
| plugins/pipelinesteps/plugin_test.go | Updates step factory expectations after step.s3_upload removal. |
| module/s3_storage.go | Deletes in-core S3 storage module implementation. |
| module/s3_storage_test.go | Deletes tests for the removed in-core S3 storage module. |
| module/pipeline_step_s3_upload.go | Deletes in-core S3 upload pipeline step implementation. |
| module/pipeline_step_s3_upload_test.go | Deletes tests for the removed S3 upload step. |
| module/iac_state_spaces.go | Deletes in-core Spaces (S3-compatible) IaC state store implementation. |
| module/iac_state_spaces_test.go | Deletes tests for the removed Spaces IaC state store. |
| module/iac_module.go | Removes in-core spaces backend and updates error/help text to point to plugins. |
| module/cloud_account_integration_test.go | Removes AWS SDK-dependent integration tests tied to deleted/changed AWS surfaces. |
| module/cloud_account_aws.go | Deletes unused in-core AWS SDK AWSConfigProvider/ValidateCredentials implementation. |
| module/cloud_account_aws_creds.go | Makes AWS profile/role_arn resolvers SDK-free by writing credential_source markers + warning log. |
| module/cloud_account_aws_creds_test.go | Adds tests asserting marker behavior + warning logs for AWS profile/role_arn. |
| DOCUMENTATION.md | Removes storage.s3 and step.s3_upload entries from module/step tables. |
| docs/migrations/2026-05-15-plugin-modules-on-iac.md | Adds Phase B migration guide for plugin cutover + coordinated upgrade requirements. |
| cmd/wfctl/state_compat_test.go | Updates doc comment references after Spaces backend removal from wfctl direct-path. |
| cmd/wfctl/infra_state_store.go | Removes wfctl direct-path Spaces backend implementation. |
| .phase-b-complete | Adds marker file to enable/enforce audit checks for Phase B completion. |
Comment on lines
+79
to
81
| m.creds.Extra["credential_source"] = "profile" | ||
| logCredentialSourceMarker("aws", "profile") | ||
| return nil |
Comment on lines
+116
to
+122
| // logCredentialSourceMarker emits a single warning line when a deferred | ||
| // credential_source marker is recorded. The warning matters during the gap | ||
| // window where an old plugin version may see a marker it doesn't yet | ||
| // understand — the message tells operators where the resolution moved. | ||
| func logCredentialSourceMarker(provider, source string) { | ||
| log.Printf("workflow: %s credential_source=%q recorded; resolution deferred to plugin (decisions/0036+0038)", provider, source) | ||
| } |
| | `iac.state` `backend: s3` | (already moved in v0.53.0; no in-core impl since then) | plugin-served by [`workflow-plugin-aws`](https://github.com/GoCodeAlone/workflow-plugin-aws) `>= v1.1.0` | | ||
| | `storage.s3` module | in-core `module.S3Storage` (registered by `plugins/storage`) | plugin-native in `workflow-plugin-aws >= v1.1.0` | | ||
| | `step.s3_upload` pipeline step | in-core `module.S3UploadStep` (registered by `plugins/pipelinesteps`) | plugin-native in `workflow-plugin-aws >= v1.1.0` | | ||
| | `cloud.account` `provider: aws` + `credentialType: profile` or `role_arn` | SDK-bearing resolver loaded the profile / called `sts:AssumeRole` in-core | core records a `credential_source` marker only; the aws plugin performs SDK resolution via `awscreds.BuildAWSConfig` (decisions/0036 + 0038) | |
Comment on lines
+73
to
+88
| ### `cloud.account provider: aws` with `credentialType: profile` or `role_arn` | ||
|
|
||
| Core no longer resolves the profile or calls `sts:AssumeRole`. Instead the | ||
| resolver records `Extra["credential_source"] = "profile"` or `"role_arn"` | ||
| (plus `Extra["profile"]` / `m.creds.RoleARN` + `Extra["external_id"]`) and | ||
| logs a `workflow: aws credential_source=…` warning. | ||
|
|
||
| The aws plugin's `awscreds.BuildAWSConfig` consumes the marker at the point of | ||
| need and performs the SDK-bearing resolution in-plugin. This is a | ||
| **co-deploy** requirement: core `>= v0.53.0` AND `workflow-plugin-aws | ||
| >= v1.1.0` must be deployed together. Mixing an old plugin against new core | ||
| results in a `credential_source` marker the plugin can't interpret — the core | ||
| warning is what tells operators which side to upgrade. | ||
|
|
||
| `credentialType: static` and `credentialType: env` are unaffected — those | ||
| have always been SDK-free. |
Comment on lines
82
to
87
| } | ||
| return &fsWfctlStateStore{dir: dir}, nil | ||
|
|
||
| case "spaces": | ||
| return resolveSpacesStateStore(cfg) | ||
|
|
||
| case "postgres": | ||
| return resolvePostgresStateStore(cfg) | ||
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…+ migration doc credentials.type + wfctl actionable spaces/s3 errors
This was referenced May 15, 2026
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
Plan-2 Phase B core deletion: removes in-core duplicates of code now plugin-served by workflow-plugin-aws v1.1.0 + workflow-plugin-digitalocean v1.1.0. Both plugin releases are live (verified: aws v1.1.0 latest with 7 assets; DO v1.1.0 latest with 5 assets).
5 commits, one per task:
862a24de): delete deadmodule/cloud_account_aws.go(zero consumers post-Audit AWS SDK usage in workflow core (RBAC/secrets/artifact stay; IaC drivers reviewed for plugin move) #653)ca917337): rewriteawsProfileResolver+awsRoleARNResolverSDK-free — declare-don't-resolve + recordExtra["credential_source"] = "profile"|"role_arn"markers + log warning. The aws plugin'sawscreds.BuildAWSConfig(already shipped via workflow-plugin-aws v1.1.0) handles marker resolution in-plugin.ef951021): deletemodule/iac_state_spaces.go+ stripcase "spaces":fromiac_module.go+ dropcmd/wfctl/consumer (spacesWfctlStateStore+resolveSpacesStateStore). spaces backend now plugin-served via workflow-plugin-digitalocean v1.1.0.3e88b2fe): deletemodule/s3_storage.go+module/pipeline_step_s3_upload.go+ drop built-in registrations fromplugins/storage/plugin.go+plugins/pipelinesteps/plugin.go+ DOCUMENTATION.md update. storage.s3 + step.s3_upload now plugin-served via workflow-plugin-aws v1.1.0.68d1e99a):go mod tidy(no-op; aws-sdk-go-v2 stays for out-of-scopeprovider/aws/,plugin/rbac/aws.go,iam/aws.go,artifact/s3.go) + arm.phase-b-completemarker →audit-cloud-symbols.sh --checkenforcement enabled + Phase B migration doc atdocs/migrations/2026-05-15-plugin-modules-on-iac.md.Breaking change
Workflow ≥ v0.53.0 (carries plan-2 PR 1 SDK extension #683). Operators using `backend: spaces`, `backend: s3`, `storage.s3`, or `step.s3_upload` MUST install the corresponding plugin (workflow-plugin-aws v1.1.0 or workflow-plugin-digitalocean v1.1.0) — see migration doc for full instructions.
Rollback
Revert PR 4 → in-core spaces/s3 storage + step + SDK-bearing resolvers restore. spaces clean-break + DO v1.1.0 release roll back as MATCHED PAIR.
Test plan
🤖 Generated with Claude Code