feat: Add --format=github for terraform output and unified pkg/env package#1985
feat: Add --format=github for terraform output and unified pkg/env package#1985
Conversation
…ckage This commit adds GitHub Actions integration for terraform outputs and creates a unified environment variable formatting package to eliminate code duplication. Key changes: 1. New `pkg/env/` package for environment variable formatting: - Supports env, dotenv, bash, and github formats - Options pattern: WithUppercase(), WithFlatten() - Sorted keys for deterministic output - Shell escaping for single-quoted literals - GitHub heredoc syntax for multiline values 2. New `pkg/github/actions/env/` package: - GetOutputPath(), GetEnvPath(), GetPathPath(), GetSummaryPath() - IsGitHubActions() detection 3. `atmos terraform output --format=github`: - Writes outputs to $GITHUB_OUTPUT or --output-file - Supports --uppercase and --flatten options - Uses heredoc syntax for multiline values 4. Refactored cmd/env and pkg/terraform/output to use pkg/env: - Eliminated duplicate formatting code - Consistent behavior across all format commands Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…v directly Eliminates unnecessary indirection by having consumers import directly from pkg/github/actions/env instead of going through a thin wrapper in pkg/env. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Move GitHub Actions utilities from pkg/github/actions/env/ to pkg/github/actions/. The nested env/ subpackage was confusing since it's unrelated to pkg/env/. Changes: - Create pkg/github/actions/actions.go with path helpers - Create pkg/github/actions/format.go with GitHub formatting (heredoc) - Create pkg/github/actions/actions_test.go with comprehensive tests - Update pkg/env/formatters.go to delegate to ghactions.FormatValue() - Update cmd imports to use ghactions alias - Delete pkg/github/actions/env/ directory Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a dedicated --format=github for terraform output, new env formatting package and GitHub Actions helpers, threads an authManager through Terraform output call chain, and implements file-writing helpers and tests to support writing outputs to $GITHUB_OUTPUT or a custom file. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as terraform output cmd
participant TF as Terraform / FormatModule
participant EnvFmt as pkg/env
participant GHActions as pkg/github/actions
participant FS as File System
User->>CLI: run --format=github
CLI->>TF: GetComponentOutputs(...)
TF->>TF: collect outputs
CLI->>GHActions: GetOutputPath()
GHActions-->>CLI: path or ""
CLI->>EnvFmt: FormatData(outputs, FormatGitHub, opts...)
EnvFmt-->>CLI: formatted string (HEREDOCs, JSON for complex)
CLI->>GHActions: FormatValue(key,value) (ensure HEREDOC delimiter)
CLI->>FS: WriteToFile(path, content)
FS-->>CLI: write result
CLI-->>User: success / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1985 +/- ##
==========================================
+ Coverage 75.10% 75.17% +0.07%
==========================================
Files 777 784 +7
Lines 71636 71802 +166
==========================================
+ Hits 53803 53978 +175
+ Misses 14347 14346 -1
+ Partials 3486 3478 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Add flexible export control for bash format output: - `atmos env --format=bash` outputs `export KEY='value'` (default) - `atmos env --format=bash --export=false` outputs `KEY='value'` Changes: - Add WithExport(bool) option to pkg/env for controlling export prefix - Add ParseFormat() function for cleaner format string conversion - Simplify cmd/env/env.go by removing duplicate switch statements - Add comprehensive tests for new functionality Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace custom EscapeSingleQuotes with battle-tested shellescape library: - shellescape.Quote handles all edge cases properly - Only quotes values when needed (simple values stay unquoted) - Uses `'"'"'` pattern for single quote escaping - Remove unused EscapeSingleQuotes function Output format changes: - Simple values: `export FOO=bar` (was: `export FOO='bar'`) - Quoted values: `export MSG='it'"'"'s'` (was: `export MSG='it'\''s'`) Both formats are valid shell syntax. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/terraform/output/executor.go (1)
178-205: ValidateauthManagertype inGetAllOutputsfor consistency.
GetOutputchecksauthManagerand returns a typed error;GetAllOutputsshould do the same to fail fast with a clear message.🐛 Suggested fix
func (e *Executor) GetAllOutputs( atmosConfig *schema.AtmosConfiguration, component string, stack string, skipInit bool, authManager any, ) (map[string]any, error) { defer perf.Track(atmosConfig, "output.Executor.GetAllOutputs")() + if authManager != nil { + if _, ok := authManager.(auth.AuthManager); !ok { + return nil, fmt.Errorf("%w: expected auth.AuthManager", errUtils.ErrInvalidAuthManagerType) + } + } + stackSlug := fmt.Sprintf("%s-%s", stack, component)pkg/terraform/output/format_test.go (3)
78-91: Test assertions mismatch implementation - CI is failing.Pipeline failures indicate the
dotenvformat implementation no longer wraps simple values in single quotes. The test expectsurl='https://example.com'but the actual output isurl=https://example.com.Either the tests need updating to match the new implementation, or the implementation has a bug. Please reconcile.
🔧 If implementation changed intentionally, update tests:
func TestFormatOutputs_Dotenv(t *testing.T) { // ... - assert.Contains(t, result, "url='https://example.com'\n") - assert.Contains(t, result, "port='8080'\n") - assert.Contains(t, result, "enabled='true'\n") + assert.Contains(t, result, "url=https://example.com\n") + assert.Contains(t, result, "port=8080\n") + assert.Contains(t, result, "enabled=true\n") }
93-106: Bash format test failures - same quoting mismatch.Same issue: tests expect
export url='https://example.com'but implementation producesexport url=https://example.com.🔧 Update if quoting behavior changed:
func TestFormatOutputs_Bash(t *testing.T) { // ... - assert.Contains(t, result, "export url='https://example.com'\n") - assert.Contains(t, result, "export port='8080'\n") - assert.Contains(t, result, "export enabled='true'\n") + assert.Contains(t, result, "export url=https://example.com\n") + assert.Contains(t, result, "export port=8080\n") + assert.Contains(t, result, "export enabled=true\n") }
362-392: Table-driven scalar tests need updates for dotenv/bash.Lines 379-380 expect quoted values for
dotenvandbashformats but CI shows unquoted output. Update these test cases to match implementation.🔧 Proposed fix:
- {"dotenv string", "url", "https://example.com", FormatDotenv, "url='https://example.com'\n"}, - {"bash string", "url", "https://example.com", FormatBash, "export url='https://example.com'\n"}, + {"dotenv string", "url", "https://example.com", FormatDotenv, "url=https://example.com\n"}, + {"bash string", "url", "https://example.com", FormatBash, "export url=https://example.com\n"},
🤖 Fix all issues with AI agents
In `@NOTICE`:
- Around line 951-954: Regenerate the NOTICE file because it is out of date
after adding the alessio/shellescape dependency; run the project’s notice
generator script (./scripts/generate-notice.sh) locally, review the updated
NOTICE output to ensure the new MIT entry for al.essio.dev/pkg/shellescape is
present, then commit the updated NOTICE file to the branch so the pipeline will
pass.
In `@pkg/github/actions/format.go`:
- Around line 14-21: The heredoc delimiter generation in FormatValue can collide
with the value content causing premature termination; update FormatValue to
ensure the delimiter is unique by checking that the chosen delimiter (currently
built as fmt.Sprintf("ATMOS_EOF_%s", key) stored in variable delimiter) does not
occur in value and, if it does, iterate (e.g., append a counter or random/UUID
suffix) until delimiter is absent from value, then emit the heredoc using that
unique delimiter; ensure the uniqueness check and iteration happen only when
strings.Contains(value, "\n") is true so single-line behavior is unchanged.
In `@website/blog/2026-01-16-github-output-format.mdx`:
- Around line 1-6: Replace the placeholder authors value in the frontmatter
("authors: [atmos]") with the actual contributor key from
website/blog/authors.yml (for example "osterman" or "aknysh") so the authors
field references the real content author; locate the frontmatter in the
github-output-format post and update the authors array to the correct author key
(e.g., authors: [osterman]) before merging.
🧹 Nitpick comments (8)
pkg/env/file.go (1)
17-31: Implementation is solid, but there's duplication.The function follows Go idioms with proper error wrapping and perf tracking. However,
pkg/terraform/output/output.gohas a nearly identicalWriteToFilefunction (lines 104-116). Consider consolidating to avoid maintaining two copies.pkg/env/value.go (1)
46-59: Minor edge case to consider.The comparison
v == float32(int64(v))works well for typical values, but floats larger thanint64max (~9.2e18) would overflow. This is unlikely in practice for environment variable values, so it's acceptable.pkg/github/actions/actions_test.go (2)
11-73: Prefert.Setenvwithoutos.Unsetenvin tests.
os.Getenvtreats unset and empty the same, sot.Setenv("", "")is enough and keeps env management scoped to the test.Based on learnings, prefer `t.Setenv` for test-scoped env changes.♻️ Suggested cleanup
t.Run("returns false when not in CI", func(t *testing.T) { // Unset by setting to empty value via t.Setenv. t.Setenv("GITHUB_ACTIONS", "") - os.Unsetenv("GITHUB_ACTIONS") assert.False(t, IsGitHubActions()) }) @@ t.Run("returns empty when not set", func(t *testing.T) { t.Setenv("GITHUB_OUTPUT", "") - os.Unsetenv("GITHUB_OUTPUT") assert.Empty(t, GetOutputPath()) }) @@ t.Run("returns empty when not set", func(t *testing.T) { t.Setenv("GITHUB_ENV", "") - os.Unsetenv("GITHUB_ENV") assert.Empty(t, GetEnvPath()) }) @@ t.Run("returns empty when not set", func(t *testing.T) { t.Setenv("GITHUB_PATH", "") - os.Unsetenv("GITHUB_PATH") assert.Empty(t, GetPathPath()) }) @@ t.Run("returns empty when not set", func(t *testing.T) { t.Setenv("GITHUB_STEP_SUMMARY", "") - os.Unsetenv("GITHUB_STEP_SUMMARY") assert.Empty(t, GetSummaryPath()) })
29-135: Consider table-driven tests for the repeated getter/format cases.
It would reduce duplication across similart.Runblocks and make it easier to extend scenarios.Based on learnings, prefer table-driven tests for multi-scenario coverage.
pkg/terraform/output/format_test.go (1)
953-966: Minor: unused assignment in helper function.Line 958 assigns
idxbut it's never used before being overwritten in the loop. Line 964 uses_ = idxto suppress warnings, but the assignment itself is unnecessary.✨ Cleaner helper:
func findSubstringIndex(s, substr string, startIdx int) int { if startIdx >= len(s) { return -1 } - idx := len(s[:startIdx]) + len(substr) for i := startIdx; i <= len(s)-len(substr); i++ { if s[i:i+len(substr)] == substr { return i } } - _ = idx // Avoid unused variable warning. return -1 }pkg/env/options.go (1)
17-23: Consider: perf.Track in option constructors.These option constructors are typically trivial and called once per formatting operation. The
perf.Trackcalls add overhead for what are essentially simple function returns. This is fine if tracing granularity is desired, but could be omitted per the guideline exceptions for "trivial getters/setters" and "simple factories."cmd/env/env_test.go (1)
269-307: TestOutputEnvAsJSON verifies JSON output path.The test initializes I/O context and verifies the JSON output path doesn't error. Consider adding assertions on actual output content for stronger coverage.
pkg/terraform/output/get_test.go (1)
112-136: LGTM!Test updated correctly for the new signature. The mock setup and assertions look good.
One thought: since
authManageris now part of the API, you might want to add a test case that verifies behavior when a non-nilauthManageris passed, especially if it affects the output retrieval flow.
Update test expectations to match shellescape.Quote behavior: - Simple values like "vpc-123", "localhost" are not quoted - Only values requiring escaping get quotes Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Regenerate NOTICE file with updated dependencies - Fix heredoc delimiter collision in FormatValue by adding suffix iteration - Add tests for delimiter collision avoidance - Update blog post author from 'atmos' to 'osterman' Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update label to be more descriptive and action-oriented - Fuse description and benefits for clearer messaging - Emphasize multiline value support without jq/manual heredocs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@website/src/data/roadmap.js`:
- Around line 321-326: The shipped milestone object for "Easily share Terraform
outputs between GitHub Actions steps" is missing the PR number; update that
milestone (the object with label 'Easily share Terraform outputs between GitHub
Actions steps', status 'shipped') to include a pr property with the value 1985
(e.g., add pr: 1985 alongside docs/changelog/version fields) so the shipped
entry is traceable and consistent with roadmap policy.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
💥 This pull request now has conflicts. Could you fix it @osterman? 🙏 |
Merge main branch to incorporate recent changes including: - GitHub Actions PATH integration (PR #1979) - Locals YAML functions support - Toolchain improvements - Various bug fixes and enhancements Resolved conflicts in: - website/plugins/fetch-latest-release/index.js - website/src/data/roadmap.js Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/env/env.go`:
- Around line 103-107: Replace raw returns of errors from envfmt.FormatData and
envfmt.WriteToFile with the project's standard/static error builder so errors
are wrapped with context; e.g., when calling envfmt.FormatData(dataMap,
envfmt.FormatGitHub) wrap the error as a static-wrapped error (include context
like "formatting env data") instead of returning err directly, and do the same
for the envfmt.WriteToFile(path, formatted) call (include context like "writing
env file"); apply the same wrapping change for the similar block around lines
111-120.
In `@cmd/terraform/output_test.go`:
- Around line 348-394: Replace OS-specific string concatenation when building
test file paths by using filepath.Join: change all occurrences that create
outputFile as tmpDir + "/github_output" to filepath.Join(tmpDir,
"github_output") in the subtests that call executeGitHubOutput (the tests that
create tmpDir and call executeGitHubOutput with tfoutput.FormatOptions), add
"path/filepath" to the test imports, and modify the "write to file error"
subtest to use a temp-dir-based non-writable path (e.g., join t.TempDir() with a
filename in a non-creatable nested directory) instead of the absolute
"/nonexistent/dir/file.txt" so the test is cross-platform and still triggers the
expected open/write error.
🧹 Nitpick comments (1)
cmd/terraform/output_test.go (1)
348-383: Strengthen GitHub output tests by asserting file contents.These subtests only assert “no error,” so a no-op write would still pass. Consider reading the output file and checking for expected keys/values (or heredoc markers) in at least one subtest to validate real output.
As per coding guidelines, tests should be comprehensive.
|
These changes were released in v1.204.1-rc.4. |
2 participants
what
pkg/env/package for environment variable formatting (env, dotenv, bash, github formats)pkg/github/actions/env/package with GitHub Actions environment file helpers--format=githubsupport foratmos terraform outputthat writes to$GITHUB_OUTPUTcmd/envandpkg/terraform/outputby refactoring to usepkg/envwhy
references
Part of environment variable formatting consolidation effort.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.