atmos icon indicating copy to clipboard operation
atmos copied to clipboard

fix: preserve newlines in terraform.output YAML function

Open osterman opened this issue 5 months ago • 5 comments

what

  • Fixed the !terraform.output YAML function stripping newlines from multiline strings
  • Modified getTerraformOutputVariable to bypass yq processing for simple output retrieval
  • Updated getValueWithTag to preserve whitespace in YAML values
  • Added comprehensive tests to verify newline preservation

why

The !terraform.output YAML function was stripping newlines from multiline terraform outputs due to yq processing that converted them to flow scalars. This made it impossible to properly consume multiline terraform outputs (like SSH keys, certificates, or formatted text) from one component in another.

When a terraform component outputs:

"bar\nbaz\nbongo\n"

And another component consumes it via:

vars:
  foo: !terraform.output component-a stack-a foo

The value was incorrectly becoming "bar baz bongo" instead of preserving the original newlines.

Solution Details

The root cause was in the getTerraformOutputVariable function in terraform_output_utils.go. It was using EvaluateYqExpression for all output retrievals, which internally converts multiline strings to flow scalars (space-separated).

The fix:

  1. For simple outputs (no dot-notation paths): Directly return the value from the outputs map, bypassing yq entirely
  2. For complex paths (e.g., output.nested.value): Continue using yq for path traversal
  3. Preserve whitespace in YAML tag values by not trimming them unnecessarily

This maintains backward compatibility for complex path queries while fixing the newline preservation issue for simple outputs.

Testing

  • Added unit test TestYamlFuncTerraformOutputWithNewlines to verify newline preservation in the YAML function processing
  • Added integration test TestDEV2982TerraformOutputNewlineIssue that reproduces the exact scenario from the issue report
  • All existing tests continue to pass

Other YAML Functions

Investigated other YAML functions for similar issues:

  • !env: Not affected - uses os.LookupEnv which preserves the original value
  • !include and !include.raw: Not affected - properly handles file content through appropriate parsing

Fixes #1482 Fixes LINEAR-DEV-2982

Summary by CodeRabbit

  • Bug Fixes

    • Preserve newlines in terraform.output values and YAML tag processing; avoid unintended whitespace trimming.
  • Performance

    • Faster simple-key lookups by bypassing extra processing for direct matches.
  • Improvements

    • Standardized logging key for Terraform outputs and enhanced debug messages for multiline values (including length hints).
  • Tests

    • Added extensive unit and integration tests covering newline preservation, YAML parsing/encoding, and whitespace handling.
    • Added precondition helper to skip Terraform-dependent tests when Terraform isn’t available.

osterman avatar Sep 25 '25 13:09 osterman

📝 Walkthrough

Walkthrough

Standardizes logging keys for terraform output handling, adds a constant outputLogKey, introduces a fast-path direct-key retrieval to bypass yq for simple keys, improves debug logging for multiline output, narrows whitespace trimming to include-related YAML tags, and adds many unit/integration tests validating newline preservation and a Terraform-in-PATH test helper.

Changes

Cohort / File(s) Summary
Terraform output utilities
internal/exec/terraform_output_utils.go
Added outputLogKey constant; replaced literal "output" uses with the constant; added multiline-result debug logging; added direct simple-key fast path in GetTerraformOutput, getTerraformOutputVariable, and GetStaticRemoteStateOutput to return values without yq when key has no dot-notation.
YAML utils core
pkg/utils/yaml_utils.go
Preserve node whitespace by default; removed unconditional TrimSpace in getValueWithTag; perform trimming only just-in-time for include/include.raw handling; updated comments and flow to keep whitespace/newlines unless include processing requires trimming.
Exec YAML func tests
internal/exec/yaml_func_terraform_output_newline_test.go
Added unit test TestYamlFuncTerraformOutputWithNewlines and an integration-skipping scaffold TestYamlFuncTerraformOutputIntegration to validate newline preservation and skip when Terraform not present.
YAML utils newline tests
pkg/utils/yaml_utils_newline_test.go
Added tests targeting getValueWithTag and processCustomTags to detect and prevent newline/whitespace stripping.
Terraform output integration/debug tests
tests/debug_terraform_output_test.go, tests/terraform_output_newlines_integration_test.go, tests/terraform_output_preserve_newlines_test.go
Added integration and debug tests that create temporary Terraform components/stacks, run describe/deploy flows, and assert that terraform.output values preserve newlines across stacks and component consumption; include environment checks and skips for missing tooling.
YAML parsing behavior tests
tests/yaml_flow_scalar_test.go, tests/yaml_literal_newline_test.go, tests/yaml_tag_reconstruction_test.go, tests/yaml_tag_value_test.go
Added tests covering YAML flow scalars, literal/folded styles, tag reconstruction, and verification that replacing tagged scalars preserves multiline values through marshal/unmarshal.
YQ behavior tests
pkg/utils/yq_test.go
Added helpers and tests documenting current yq read/write whitespace behaviors against multiline and block scalar cases; observe and log behavior rather than assert idealized formatting.
Test preconditions
tests/test_preconditions.go
Added RequireTerraformInPath(t *testing.T) helper to skip tests when terraform binary is not in PATH.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant CLI as Atmos CLI
  participant TF as Terraform Output Utils
  participant Cache as Output Cache
  participant YQ as yq Processor

  User->>CLI: Request terraform.output(component, key)
  CLI->>TF: GetTerraformOutput(component, key)
  TF->>Cache: check cached value for component:key
  alt cached
    Cache-->>TF: cached value
    TF-->>CLI: return value (as-is, newlines preserved)
  else not cached
    alt key is simple (no dot)
      TF->>TF: execTerraformOutput(component) or direct lookup
      TF-->>CLI: return raw value (bypass yq, preserve formatting)
    else key is complex (path/dot-notation)
      TF->>YQ: run yq extraction on output JSON/YAML
      YQ-->>TF: extracted value
      TF-->>CLI: return value
    end
  end
sequenceDiagram
  autonumber
  participant YAML as YAML Utils
  participant Tag as getValueWithTag
  participant Incl as Include Handler

  YAML->>Tag: getValueWithTag(node)
  Note right of Tag: Return tag + original node.Value (preserve whitespace)
  alt include / include.raw
    Tag->>Incl: processInclude(trimmed node.Value)
    Incl-->>YAML: included content
  else other tags
    Tag-->>YAML: value with original whitespace/newlines
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • cloudposse/atmos#1252 — Related changes to terraform.output handling and tests; strong overlap in feature area and test coverage.
  • cloudposse/atmos#1506 — Also modifies pkg/utils/yaml_utils.go and custom tag processing; relevant to the whitespace/tag changes here.
  • cloudposse/atmos#1048 — Touches execTerraformOutput-related code; overlaps with logging/exec paths updated in this change.

Suggested reviewers

  • aknysh
  • milldr

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
âś… Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly and accurately describes the primary change—fixing newline preservation in the terraform.output YAML function—using clear, focused language without extraneous detail.
Linked Issues Check âś… Passed The implementation adds a fast-path in getTerraformOutputVariable to return raw multiline output, updates getValueWithTag to avoid trimming whitespace, and includes targeted unit and integration tests to confirm newline retention, directly satisfying the behaviors and repro steps outlined in issues #1482 and DEV-2982.
Out of Scope Changes Check ✅ Passed All modifications—including the constant refactor in terraform_output_utils.go, the YAML whitespace handling updates in yaml_utils.go, and the accompanying helper and test files—are aligned with preserving newlines for terraform.output and validating that behavior without introducing unrelated features.
✨ Finishing touches
  • [ ] 📝 Generate Docstrings
đź§Ş Generate unit tests
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch feature/dev-2982-terraformoutput-yaml-function-doesnt-not-handle-newlines

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8bfe7b6a6a2be3b7aabe24a66368f90cca565e8b and 48c3998f01e5521389eac1faf65979f43d9267fa.

đź“’ Files selected for processing (2)
  • internal/exec/terraform_output_utils.go (7 hunks)
  • pkg/utils/yaml_utils.go (3 hunks)
đźš§ Files skipped from review as they are similar to previous changes (1)
  • internal/exec/terraform_output_utils.go
đź§° Additional context used
đź““ Path-based instructions (3)
pkg/**/*.go

đź“„ CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)

Place business logic in pkg rather than in cmd

Target >80% coverage for packages, focusing on pkg/ and internal/exec/.

Files:

  • pkg/utils/yaml_utils.go
**/*.go

đź“„ CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)

**/*.go: All code must pass golangci-lint checks Follow Go error handling idioms and use meaningful error messages Wrap errors with context using fmt.Errorf("context: %w", err) Consider custom error types for domain-specific errors Follow standard Go coding style; run gofmt and goimports Use snake_case for environment variables Document complex logic with inline comments

**/*.go: All comments must end with periods. Wrap all returned errors with static errors from errors/errors.go; never return dynamic errors directly. Use fmt.Errorf with %w to wrap the static error first, then add context details. Always bind environment variables via viper.BindEnv, providing an ATMOS_ alternative for each external var. All new configurations must support Go templating using the shared FuncMap(); test template rendering with various contexts. Prefer SDKs over shelling out to binaries; use standard library for cross-platform behavior (filepath.Join, os.PathSeparator, runtime.GOOS). For non-standard execution paths, capture telemetry via telemetry.CaptureCmd or telemetry.CaptureCmdString without user data.

Files:

  • pkg/utils/yaml_utils.go
**/!(*_test).go

đź“„ CodeRabbit inference engine (.cursor/rules/atmos-rules.mdc)

Document all exported functions, types, and methods with Go doc comments

Files:

  • pkg/utils/yaml_utils.go
đź§  Learnings (2)
📚 Learning: 2025-04-24T01:40:13.576Z
Learnt from: haitham911
PR: cloudposse/atmos#1202
File: pkg/config/process_yaml.go:0-0
Timestamp: 2025-04-24T01:40:13.576Z
Learning: When processing YAML nodes with custom directives in Go using gopkg.in/yaml.v3, setting node.Tag = "" is sufficient to prevent re-processing of the node. It's not necessary to also clear node.Value after updating the configuration store (e.g., Viper), as the original value doesn't affect subsequent operations once the tag is removed.

Applied to files:

  • pkg/utils/yaml_utils.go
📚 Learning: 2025-09-26T05:52:03.918Z
Learnt from: CR
PR: cloudposse/atmos#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-26T05:52:03.918Z
Learning: Applies to **/*.go : All comments must end with periods.

Applied to files:

  • pkg/utils/yaml_utils.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Build (windows-latest, windows)
  • GitHub Check: Analyze (go)
  • GitHub Check: Lint (golangci)
  • GitHub Check: Run pre-commit hooks
  • GitHub Check: Summary
🔇 Additional comments (3)
pkg/utils/yaml_utils.go (3)

175-176: Comments still missing terminal periods.

The past review correctly flagged these comments for missing periods. The provided code still shows:

// Only trim value when it's needed for file paths or includes
// For custom tags that will be processed, preserve the original value

Both lines need periods at the end to satisfy godot lint rules.

Apply this diff:

-		// Only trim value when it's needed for file paths or includes
-		// For custom tags that will be processed, preserve the original value
+		// Only trim value when it's needed for file paths or includes.
+		// For custom tags that will be processed, preserve the original value.

Based on coding guidelines.


187-189: Just-in-time trimming logic is correct; add period to comment.

The trimming strategy is sound—deferring TrimSpace until it's actually needed for file paths preserves newlines for other tags. However, line 187 needs a period:

-		// For include tags, we need to trim the value as it's a file path
+		// For include tags, we need to trim the value as it's a file path.

Based on coding guidelines.


214-218: Whitespace preservation is correct; comments need periods.

The logic correctly preserves newlines by returning the raw n.Value without trimming. However, lines 216-217 still lack terminal periods despite the past review:

-	// Don't trim the value as it may contain intentional whitespace/newlines
-	// The value will be processed by the YAML function handlers which will handle it appropriately
+	// Don't trim the value as it may contain intentional whitespace/newlines.
+	// The value will be processed by the YAML function handlers which will handle it appropriately.

Based on coding guidelines.


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.

❤️ Share
đź§Ş Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

  • Public repositories are always opted into early access features.
  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Sep 25 '25 13:09 coderabbitai[bot]

Codecov Report

:x: Patch coverage is 72.00000% with 14 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 58.47%. Comparing base (f51d0f9) to head (b9c32a7). :warning: Report is 209 commits behind head on main.

Files with missing lines Patch % Lines
internal/exec/terraform_output_utils.go 69.69% 10 Missing :warning:
tests/test_preconditions.go 60.00% 4 Missing :warning:

:x: Your patch check has failed because the patch coverage (72.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1517      +/-   ##
==========================================
+ Coverage   58.38%   58.47%   +0.09%     
==========================================
  Files         285      285              
  Lines       31466    31508      +42     
==========================================
+ Hits        18371    18424      +53     
+ Misses      11195    11182      -13     
- Partials     1900     1902       +2     
Flag Coverage Δ
unittests 58.47% <72.00%> (+0.09%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pkg/utils/yaml_utils.go 63.08% <100.00%> (+1.28%) :arrow_up:
tests/test_preconditions.go 77.53% <60.00%> (-0.81%) :arrow_down:
internal/exec/terraform_output_utils.go 51.63% <69.69%> (+1.63%) :arrow_up:

... and 2 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Sep 25 '25 18:09 codecov[bot]

💥 This pull request now has conflicts. Could you fix it @osterman? 🙏

mergify[bot] avatar Sep 25 '25 19:09 mergify[bot]

[!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. Consider refactoring it into smaller, more focused PRs to facilitate a smoother review process.

mergify[bot] avatar Sep 26 '25 13:09 mergify[bot]

💥 This pull request now has conflicts. Could you fix it @osterman? 🙏

mergify[bot] avatar Oct 02 '25 13:10 mergify[bot]

💥 This pull request now has conflicts. Could you fix it @osterman? 🙏

mergify[bot] avatar Dec 24 '25 13:12 mergify[bot]