Skip to content

[wip] fix: #4381 remove terragrunt.values.hcl file when stack no longer has values provided. #4428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wakeful
Copy link
Member

@wakeful wakeful commented Jun 16, 2025

Description

Fixes #4381 remove terragrunt.values.hcl file when stack no longer has values provided.

TODOs

Read the Gruntwork contribution guidelines.

  • I authored this code entirely myself
  • I am submitting code based on open source software (e.g. MIT, MPL-2.0, Apache)]
  • I am adding or upgrading a dependency or adapted code and confirm it has a compatible open source license
  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

fix: #4381 remove terragrunt.values.hcl file when stack no longer has values provided.

Migration Guide

n/a

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of missing values files by ensuring they are properly removed when no values are present, with clearer debug logging for missing files.
  • New Features
    • Added support for automatic removal of obsolete values files during stack generation.
    • Introduced new stack units and configurations to demonstrate message echoing with default and custom values.
  • Tests
    • Added integration tests verifying correct creation and removal of values files in different stack environments.

@wakeful wakeful requested review from denis256 and yhakbar as code owners June 16, 2025 12:06
Copy link

vercel bot commented Jun 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
terragrunt-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 18, 2025 9:18pm

Copy link
Contributor

coderabbitai bot commented Jun 16, 2025

📝 Walkthrough

Walkthrough

The writeValues function in config/stack.go was updated to remove the generated values file if values is nil. It now reads the existing values file to check if it is auto-generated before deleting it, handling errors gracefully. A new constant for the auto-generated comment was introduced and used when writing values files.

Changes

File(s) Change Summary
config/stack.go Updated writeValues to remove the values file if values is nil, with buffered reading to detect auto-generated files, added auto-generated comment constant, and improved error handling.
test/fixtures/stacks/value-block-remove/env/dev/terragrunt.stack.hcl Added a new unit block echo with source, path, and values to test removal of values block.
test/fixtures/stacks/value-block-remove/env/dev/terragrunt.stack.hcl-post Added a post-modification stack file defining unit echo without values block to test removal behavior.
test/fixtures/stacks/value-block-remove/env/qa/terragrunt.stack.hcl Added a new stack file defining unit echo with default values file to test presence of values file.
test/fixtures/stacks/value-block-remove/module/message/main.tf Added Terraform module with variable msg and output echo to support echo message functionality in tests.
test/fixtures/stacks/value-block-remove/units/echo-with-default/terragrunt.hcl Added Terragrunt config for unit with default message value using try function.
test/fixtures/stacks/value-block-remove/units/echo-with-default/terragrunt.values.hcl Added example default values file for QA environment.
test/fixtures/stacks/value-block-remove/units/echo/terragrunt.hcl Added Terragrunt config for unit with message input and default value.
test/integration_stacks_test.go Added two integration tests verifying removal and presence of values files when values block is removed or present.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant StackConfig

    Caller->>StackConfig: writeValues(values)
    alt values is nil
        StackConfig->>FileSystem: Open existing values file
        alt File exists
            StackConfig->>FileSystem: Read first line to check auto-generated comment
            alt File is auto-generated
                StackConfig->>FileSystem: Remove values file
                alt Removal success
                    StackConfig-->>Caller: Return nil
                else Removal error
                    StackConfig-->>Caller: Return error
                end
            else File is user-provided
                StackConfig->>Logger: Log debug message, skip removal
                StackConfig-->>Caller: Return nil
            end
        else File does not exist
            StackConfig->>Logger: Log debug message, skip removal
            StackConfig-->>Caller: Return nil
        end
    else
        StackConfig->>FileSystem: Write values file with auto-generated comment
        StackConfig-->>Caller: Return result
    end
Loading

Assessment against linked issues

Objective Addressed Explanation
Remove generated values file when the values block is removed from the unit ( #4381 )
Ensure that after removing the values block and running stack apply, the old values file is deleted ( #4381 )

Suggested reviewers

  • levkohimins
  • yhakbar

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (1.64.8)

Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2
Failed executing command with error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 57dc4f7 and 2fa447e.

📒 Files selected for processing (9)
  • config/stack.go (4 hunks)
  • test/fixtures/stacks/value-block-remove/env/dev/terragrunt.stack.hcl (1 hunks)
  • test/fixtures/stacks/value-block-remove/env/dev/terragrunt.stack.hcl-post (1 hunks)
  • test/fixtures/stacks/value-block-remove/env/qa/terragrunt.stack.hcl (1 hunks)
  • test/fixtures/stacks/value-block-remove/module/message/main.tf (1 hunks)
  • test/fixtures/stacks/value-block-remove/units/echo-with-default/terragrunt.hcl (1 hunks)
  • test/fixtures/stacks/value-block-remove/units/echo-with-default/terragrunt.values.hcl (1 hunks)
  • test/fixtures/stacks/value-block-remove/units/echo/terragrunt.hcl (1 hunks)
  • test/integration_stacks_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • test/fixtures/stacks/value-block-remove/units/echo/terragrunt.hcl
  • test/fixtures/stacks/value-block-remove/units/echo-with-default/terragrunt.hcl
  • test/fixtures/stacks/value-block-remove/units/echo-with-default/terragrunt.values.hcl
  • test/fixtures/stacks/value-block-remove/env/qa/terragrunt.stack.hcl
  • test/fixtures/stacks/value-block-remove/module/message/main.tf
  • test/fixtures/stacks/value-block-remove/env/dev/terragrunt.stack.hcl
  • config/stack.go
  • test/integration_stacks_test.go
  • test/fixtures/stacks/value-block-remove/env/dev/terragrunt.stack.hcl-post
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Pull Request has non-contributor approval
✨ Finishing Touches
  • 📝 Generate Docstrings

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
config/stack.go (1)

756-760: Minor: avoid joining with empty directory

When directory is empty, filepath.Join("", valuesFile) yields a relative path in the CWD.
Consider validating directory before calling os.Remove (same as you already do later for the write case) to avoid surprising deletions if the caller passes an empty string by mistake.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b0fdf2 and a7ce857.

📒 Files selected for processing (1)
  • config/stack.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.

**/*.go: Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.

  • config/stack.go
⏰ Context from checks skipped due to timeout of 90000ms (11)
  • GitHub Check: Test (Windows)
  • GitHub Check: Test (AWS with Latest Terraform)
  • GitHub Check: Test (Fixtures with Latest OSS Terraform)
  • GitHub Check: License Check
  • GitHub Check: Build (windows/amd64)
  • GitHub Check: Test OIDC (GHA AWS)
  • GitHub Check: Build (linux/amd64)
  • GitHub Check: Test (ubuntu)
  • GitHub Check: lint
  • GitHub Check: Test (macos)
  • GitHub Check: lint
🔇 Additional comments (1)
config/stack.go (1)

761-768: ```shell
#!/bin/bash

Verify the existence of the file and display the relevant lines

if [ -f config/stack.go ]; then
echo "=== config/stack.go: Lines 740-800 ==="
sed -n '740,800p' config/stack.go
else
echo "config/stack.go not found"
fi


</details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

@wakeful wakeful force-pushed the fix-4381-regenerate-values-when-values-block-was-removed branch from a7ce857 to 467f8fb Compare June 16, 2025 12:13
yhakbar
yhakbar previously approved these changes Jun 16, 2025
Copy link
Collaborator

@yhakbar yhakbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, but please add a test to confirm we don't regress on this in the future.

@wakeful wakeful force-pushed the fix-4381-regenerate-values-when-values-block-was-removed branch from 467f8fb to 57dc4f7 Compare June 18, 2025 21:07
@wakeful wakeful force-pushed the fix-4381-regenerate-values-when-values-block-was-removed branch from 57dc4f7 to 2fa447e Compare June 18, 2025 21:17
@wakeful wakeful requested a review from yhakbar June 18, 2025 21:21
@yhakbar
Copy link
Collaborator

yhakbar commented Jun 19, 2025

Should we move this in-draft, then consider closing this out if we merge #4446? It resolves the need to do this check, right?

@wakeful
Copy link
Member Author

wakeful commented Jun 19, 2025

Should we move this in-draft, then consider closing this out if we merge #4446? It resolves the need to do this check, right?

agree, let me change the status

@wakeful wakeful marked this pull request as draft June 19, 2025 12:13
@wakeful wakeful changed the title fix: #4381 remove terragrunt.values.hcl file when stack no longer has values provided. [wip] fix: #4381 remove terragrunt.values.hcl file when stack no longer has values provided. Jun 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stacks do not regenerate values if values block is removed
2 participants