wfctl secrets: use config.LoadFromFile to honor imports: directives#560
Merged
Conversation
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…port - Replace direct os.ReadFile+yaml.Unmarshal in loadSecretsConfig, loadWorkflowConfigForSecrets, and runSecretsDetect in secrets_detect.go with config.LoadFromFile so import directives are honored - Replace direct os.ReadFile+yaml.Unmarshal in runSecretsSetup in secrets_setup.go with config.LoadFromFile - Replace direct os.ReadFile+yaml.Unmarshal in parseSecretsConfig in infra_secrets.go with config.LoadFromFile - Remove now-unused gopkg.in/yaml.v3 imports from secrets_detect.go and secrets_setup.go - Add regression tests in secrets_imports_test.go covering imported entries, secretStores, defaultStore resolution, and validate behavior Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/29c5afe6-a4f0-4dce-9a30-b911e006cc4b Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix wfctl secrets commands to parse config through imports
wfctl secrets: use config.LoadFromFile to honor imports: directives
May 6, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates wfctl secrets and infra secret-sync paths to load workflow configuration via config.LoadFromFile so imports: directives are honored, ensuring secrets, secret stores, and per-environment overrides defined in imported YAML files are visible to these commands.
Changes:
- Replace
os.ReadFile+yaml.Unmarshalwithconfig.LoadFromFilein secrets-related command loaders and infra secrets parsing. - Improve missing-config handling by switching to
errors.Is(err, os.ErrNotExist)whereLoadFromFilewraps errors. - Add regression tests covering secrets/import merge behavior across wfctl secrets + infra secret parsing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/wfctl/secrets_setup.go | Loads merged config via config.LoadFromFile so secrets setup can see imported secrets/defaultStore. |
| cmd/wfctl/secrets_detect.go | Uses config.LoadFromFile for secrets config loading and missing-file fallbacks compatible with wrapped errors. |
| cmd/wfctl/infra_secrets.go | Parses secrets config via config.LoadFromFile so infra bootstrap/post-apply secret sync sees imported secrets: content. |
| cmd/wfctl/secrets_imports_test.go | Adds tests asserting imports are honored for secrets entries/defaultStore/secretStores and infra secrets parsing. |
Comment on lines
+63
to
+79
| // TestLoadSecretsConfig_MainWinsOverImport verifies that when the same entry | ||
| // is declared in both main and imported files, the main file's definition wins. | ||
| func TestLoadSecretsConfig_MainWinsOverImport(t *testing.T) { | ||
| dir := t.TempDir() | ||
|
|
||
| shared := `secrets: | ||
| defaultStore: imported-store | ||
| entries: | ||
| - name: SHARED_SECRET | ||
| ` | ||
| main := `imports: | ||
| - shared.yaml | ||
| secrets: | ||
| defaultStore: main-store | ||
| entries: | ||
| - name: MAIN_SECRET | ||
| ` |
Contributor
|
@copilot apply changes based on the comments in this thread |
- Replace resolveSecretStoreForSetup+newSecretsProvider in runSecretsSetup with ResolveSecretStore+getProviderForStore to correctly use SecretsStoreOverride (not SecretsProvider) for env-level overrides and to properly look up named stores from SecretStores map - Remove the now-redundant resolveSecretStoreForSetup function - Fix parseSecretsConfig comment: remove misleading mention of secretStores (the function only returns cfg.Secrets, not SecretStores) - Improve TestLoadSecretsConfig_MainWinsOverImport: add a duplicate entry name in both files to actually exercise conflict-resolution (main wins) and assert no duplicates in the merged result - Update TestSecretsSetup_HonorsImportedDefaultStore: remove leftover dead scaffolding (entry/_ = entry) and use ResolveSecretStore directly Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/570bc092-953b-4c28-873c-ba02086b0ad6 Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Contributor
Author
Applied all four review changes in commit 9143418:
|
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.
All
wfctl secretssubcommands (get,validate,list,set,rotate,sync,setup,detect) andinfrapost-apply secret sync were reading YAML directly viaos.ReadFile+yaml.Unmarshal, skippingconfig.LoadFromFile's import-merge pipeline. Secrets, secret stores, and per-environment overrides declared only in imported files were invisible to these commands.Changes
secrets_detect.go—runSecretsDetect,loadSecretsConfig,loadWorkflowConfigForSecrets: replace direct unmarshal withconfig.LoadFromFile; missing-file fallbacks now useerrors.Is(err, os.ErrNotExist)to handle the wrapped error; remove unusedyamlimport.secrets_setup.go—runSecretsSetup: replace direct unmarshal withconfig.LoadFromFile; replaceresolveSecretStoreForSetup+newSecretsProviderwithResolveSecretStore+getProviderForStoreso environment-level overrides correctly useSecretsStoreOverride(not the unrelatedSecretsProviderfield) and named stores are looked up fromSecretStores; remove the now-redundantresolveSecretStoreForSetupfunction; remove unusedyamlimport.infra_secrets.go—parseSecretsConfig: replace direct unmarshal withconfig.LoadFromFileso infra bootstrap and post-apply secret sync see importedgenerate[]entries and stores.Example — previously broken
Before:
wfctl secrets get API_TOKENfell back to theenvprovider (emptysecrets:block). After: resolves to thevaultstore via the merged config.Regression tests (
secrets_imports_test.go)loadSecretsConfigsurfaces imported entries anddefaultStore; main-file wins on conflicts, including duplicate entry names (no duplicates in merged result).loadWorkflowConfigForSecretssurfaces importedsecretStores.parseSecretsConfigsurfaces importedentriesandgenerate[].runSecretsValidatepasses/fails correctly against imported entries.ResolveSecretStorepicks the importeddefaultStoreforsecrets setup.