Repository navigation
feat: add workflow environment variable support with hierarchical merging - #2050
Merged
Merged
Conversation
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
|
Warning Release Documentation RequiredThis PR is labeled
|
Member
|
Add the missing documentation as well for the new configuration setting. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 342ea06701
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorkflow and step-scoped environment maps were added and merged hierarchically. Execution now builds a reusable baseEnv (system + atmos/toolchain PATH), merges workflow.Env and step.Env (step wins), and authentication appends identity env vars onto the merged environment before step execution. PrepareEnvironment now always merges provided baseEnv onto os.Environ(). Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Executor as Executor
participant OS as OS
participant Auth as AuthManager
participant Runner as Runner
User->>Executor: request run step (workflow.Env, step.Env, identity?)
Executor->>OS: read os.Environ()
OS-->>Executor: system env
Executor->>Executor: baseEnv = system env + atmos/toolchain PATH
Executor->>Executor: mergedEnv = merge(workflow.Env, step.Env) -- step wins
Executor->>Auth: prepareStepEnvironment(baseEnv, identity?, mergedEnv)
alt identity provided and auth provider exists
Auth-->>Executor: identity env vars
Executor->>Executor: finalEnv = baseEnv + mergedEnv + identity env
else
Executor->>Executor: finalEnv = baseEnv + mergedEnv
end
Executor->>Runner: execute step with finalEnv
Runner-->>User: step result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
📝 WalkthroughWalkthroughThis pull request introduces environment variable support at both workflow and step levels in workflow execution. Variables are hierarchically merged with step-level variables overriding workflow-level duplicates for the same keys. The feature is implemented across schema definitions, execution logic, comprehensive tests, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Executor
participant prepareStepEnvironment
participant mergeWorkflowEnv
participant Runner
Executor->>prepareStepEnvironment: Call with workflowEnv, stepEnv
prepareStepEnvironment->>mergeWorkflowEnv: Merge workflow and step vars
mergeWorkflowEnv-->>prepareStepEnvironment: Return merged map (step overrides)
prepareStepEnvironment->>prepareStepEnvironment: Build final env slice<br/>(system → global → merged)
prepareStepEnvironment-->>Executor: Return env slice
Executor->>Runner: Execute with final environment<br/>(system + global + workflow + step)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/exec/workflow_utils.go`:
- Around line 123-150: The prepareStepEnvironment function currently calls
os.Environ() internally which ignores the single consolidated base environment
and PATH/toolchain setup; change prepareStepEnvironment signature to accept
baseEnv []string instead of calling os.Environ(), update ExecuteWorkflow to
construct baseEnv once via envpkg.MergeGlobalEnv(os.Environ(), globalEnv) and
pass that baseEnv into prepareStepEnvironment for non-identity steps, and for
identity-bearing steps construct baseEnv inside the auth flow before adding
credentials; also update all callers of prepareStepEnvironment and preserve the
existing mergedEnv logic (workflowEnvMap + stepEnvMap overriding) and appending
envpkg.ConvertMapToSlice(mergedEnv) to the provided baseEnv.
🧹 Nitpick comments (1)
internal/exec/workflow_utils_test.go (1)
978-1071: Consider collapsing the env cases into a table‑driven test. These scenarios are structurally similar and could be expressed as a single table for easier extension. As per coding guidelines: Use table-driven tests for testing multiple scenarios in Go.
Contributor
Author
|
Documentation added in commit 31c5c4e. Updated
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/exec/workflow_utils.go`:
- Around line 181-186: The auth-env preparation error returned from
authManager.PrepareShellEnvironment should be wrapped with the static sentinel
(errUtils.ErrAuthenticationFailed) so callers can reliably use errors.Is; update
the error return to combine the sentinel and the original authErr (e.g. combined
:= errors.Join(errUtils.ErrAuthenticationFailed, authErr)) and then wrap that
combined error in the existing fmt.Errorf context for PrepareShellEnvironment
(or use fmt.Errorf with %w), ensuring you import the standard errors package and
reference authManager.PrepareShellEnvironment and
errUtils.ErrAuthenticationFailed.
🧹 Nitpick comments (1)
internal/exec/workflow_utils_test.go (1)
833-1043: Consolidate the prepareStepEnvironment variants into a table-driven test.These scenarios are all the same behavior with different inputs; a table makes this easier to extend and maintain.
♻️ Suggested refactor
-// TestPrepareStepEnvironment tests the prepareStepEnvironment function. -func TestPrepareStepEnvironment_NoIdentity(t *testing.T) { - // When no identity is specified, should return base environment with workflow/step env merged. - baseEnv := []string{"BASE_VAR=base-value"} - env, err := prepareStepEnvironment(baseEnv, "", "step1", nil, nil, nil) - - assert.NoError(t, err) - // Should return the base environment. - assert.Contains(t, env, "BASE_VAR=base-value") -} - -// TestPrepareStepEnvironment_NilAuthManager tests prepareStepEnvironment with nil auth manager. -func TestPrepareStepEnvironment_NilAuthManager(t *testing.T) { - baseEnv := []string{"BASE_VAR=base-value"} - env, err := prepareStepEnvironment(baseEnv, "some-identity", "step1", nil, nil, nil) - - assert.ErrorIs(t, err, errUtils.ErrAuthManager) - assert.Nil(t, env) -} +func TestPrepareStepEnvironment_NoIdentity(t *testing.T) { + tests := []struct { + name string + baseEnv []string + workflowEnv map[string]string + stepEnv map[string]string + expectEmpty bool + expectContains []string + expectNotContains []string + }{ + { + name: "base only", + baseEnv: []string{"BASE_VAR=base-value"}, + expectContains: []string{"BASE_VAR=base-value"}, + }, + { + name: "empty base", + baseEnv: []string{}, + expectEmpty: true, + }, + { + name: "workflow env", + baseEnv: []string{"BASE_VAR=base-value"}, + workflowEnv: map[string]string{"WORKFLOW_VAR": "workflow-value"}, + expectContains: []string{"BASE_VAR=base-value", "WORKFLOW_VAR=workflow-value"}, + }, + { + name: "step env", + baseEnv: []string{"BASE_VAR=base-value"}, + stepEnv: map[string]string{"STEP_VAR": "step-value"}, + expectContains: []string{"BASE_VAR=base-value", "STEP_VAR=step-value"}, + }, + { + name: "step overrides workflow", + baseEnv: []string{"BASE_VAR=base-value"}, + workflowEnv: map[string]string{"MY_VAR": "workflow-value"}, + stepEnv: map[string]string{"MY_VAR": "step-value"}, + expectContains: []string{"MY_VAR=step-value"}, + expectNotContains: []string{"MY_VAR=workflow-value"}, + }, + { + name: "merge workflow and step", + baseEnv: []string{"BASE_VAR=base-value"}, + workflowEnv: map[string]string{"WORKFLOW_VAR": "workflow-value"}, + stepEnv: map[string]string{"STEP_VAR": "step-value"}, + expectContains: []string{"BASE_VAR=base-value", "WORKFLOW_VAR=workflow-value", "STEP_VAR=step-value"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + env, err := prepareStepEnvironment(tt.baseEnv, "", "step1", nil, tt.workflowEnv, tt.stepEnv) + assert.NoError(t, err) + if tt.expectEmpty { + assert.Empty(t, env) + return + } + for _, expected := range tt.expectContains { + assert.Contains(t, env, expected) + } + for _, unexpected := range tt.expectNotContains { + assert.NotContains(t, env, unexpected) + } + }) + } +} + +// TestPrepareStepEnvironment_NilAuthManager tests prepareStepEnvironment with nil auth manager. +func TestPrepareStepEnvironment_NilAuthManager(t *testing.T) { + baseEnv := []string{"BASE_VAR=base-value"} + env, err := prepareStepEnvironment(baseEnv, "some-identity", "step1", nil, nil, nil) + + assert.ErrorIs(t, err, errUtils.ErrAuthManager) + assert.Nil(t, env) +}As per coding guidelines, Use table-driven tests for testing multiple scenarios in Go.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2050 +/- ##
==========================================
+ Coverage 76.08% 76.10% +0.01%
==========================================
Files 797 797
Lines 74625 74649 +24
==========================================
+ Hits 56780 56808 +28
+ Misses 14302 14296 -6
- Partials 3543 3545 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Refactors the legacy executor's prepareStepEnvironment function to accept baseEnv []string as a parameter instead of calling os.Environ() internally. Changes: - prepareStepEnvironment now takes baseEnv as first parameter - Removes globalEnv parameter (caller merges it into baseEnv) - ExecuteWorkflow constructs baseEnv once before the step loop - Toolchain PATH is included in baseEnv upfront - Updated all test calls to pass baseEnv parameter Benefits: - Aligns with modern executor pattern - Improves testability (tests can pass controlled base env) - More efficient (base env constructed once instead of per-step) - Better PR #1899 compatibility for step handler architecture Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Wrap the error from authManager.PrepareShellEnvironment with errUtils.ErrAuthenticationFailed so callers can reliably use errors.Is() to detect authentication failures. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add clear documentation explaining that workflow environment variables are scoped to individual steps: - Workflow-level env applies to all steps - Step-level env only applies to that specific step - Step-level env does NOT persist to subsequent steps - Shell exports within a step do NOT persist to subsequent steps Also adds an env-scoping test case to verify this behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add env-scoping to the workflow_not_found snapshot after adding the new scoping test case to the fixtures. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Reduce verbose examples to a single concise example showing both workflow and step level env usage. Keep essential info: precedence order and scoping behavior. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Ben (Benbentwo)
force-pushed
the
Benbentwo/workflow-env-vars
branch
from
February 5, 2026 18:24
b04d474 to
d929213
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/exec/workflow_adapters_test.go`:
- Around line 551-552: Update the inline comment that reads "Adapter now always
merges baseEnv with os.Environ(), so we use gomock.Any() and verify the behavior
rather than exact env slice" to end with a period to satisfy the godot linter;
locate this comment in the workflow_adapters_test.go test (near the gomock.Any()
usage in the test that verifies env merging) and append a trailing period to the
comment line.
In `@internal/exec/workflow_adapters.go`:
- Around line 103-106: The comment above the fullEnv initialization is missing a
terminal period and will fail the godot linter; update the comment sentences in
the block that references os.Environ() and fullEnv so each ends with a period
(e.g., ensure the two comment lines both terminate with a period).
🧹 Nitpick comments (2)
website/docs/cli/configuration/workflows.mdx (1)
144-160: Consider wrapping the YAML example in a<File>component for consistency.Other workflow YAML examples in this file use the
<File title="...">wrapper (e.g., lines 55, 87, 105, 126). This bare code block breaks the visual pattern.📝 Suggested change
-```yaml -workflows: - deploy-multi-region: - env: - TF_LOG: INFO - steps: - - command: terraform apply vpc -s plat-ue2-dev --auto-approve - env: - AWS_REGION: us-east-2 - - command: terraform apply vpc -s plat-uw2-dev --auto-approve - env: - AWS_REGION: us-west-2 -``` +<File title="stacks/workflows/deploy.yaml"> +```yaml +workflows: + deploy-multi-region: + env: + TF_LOG: INFO + steps: + - command: terraform apply vpc -s plat-ue2-dev --auto-approve + env: + AWS_REGION: us-east-2 + - command: terraform apply vpc -s plat-uw2-dev --auto-approve + env: + AWS_REGION: us-west-2 +``` +</File>internal/exec/workflow_utils_test.go (1)
1130-1160: Align the fallback test name/comments with what it actually executes.The test runs a well‑formed command, so it doesn’t exercise the fallback path and reads as misleading. Consider renaming and clarifying comments to avoid a tautological test.
As per coding guidelines Test behavior, not implementation - Never test stub functions - Avoid tautological tests - Use `errors.Is()` for error checking - Remove always-skipped tests.🔧 Clarify intent and naming
-// TestExecuteWorkflow_ShellFieldsFallback tests that ExecuteWorkflow falls back to -// strings.Fields when shell.Fields fails to parse the command. -func TestExecuteWorkflow_ShellFieldsFallback(t *testing.T) { +// TestExecuteWorkflow_ShellFieldsNormalParse ensures ExecuteWorkflow succeeds with a +// well-formed command (fallback coverage is in TestExecuteWorkflow_ShellFieldsFallbackWithMalformedCommand). +func TestExecuteWorkflow_ShellFieldsNormalParse(t *testing.T) { @@ - // Use a command with unclosed quote that shell.Fields can't parse. - // This tests the fallback path to strings.Fields. - // Note: The command itself will fail when executed, but we're testing - // that the parsing fallback works correctly. + // Use a well-formed command. + // Malformed fallback is covered in TestExecuteWorkflow_ShellFieldsFallbackWithMalformedCommand.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
approved these changes
Feb 9, 2026
Erik Osterman (Cloud Posse) (osterman)
approved these changes
Feb 9, 2026
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
website/src/data/roadmap.js (1)
252-255:⚠️ Potential issue | 🟡 MinorInitiative marked 100% shipped but contains an in-progress milestone.
Line 253 sets
progress: 100andstatus: 'shipped'for the Workflows Overhaul initiative, yet line 255 still has "New workflow step types" asstatus: 'in-progress'. Either bump that milestone toshippedor adjust the initiative progress/status to reflect the remaining work.
Andriy Knysh (aknysh)
approved these changes
Feb 9, 2026
|
These changes were released in v1.206.0-rc.4. |
This branch was successfully deployed
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
3 participants
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.
what
envmap support at workflow and step levels in workflow YAML fileswhy
stackfields workreferences
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests