atmos icon indicating copy to clipboard operation
atmos copied to clipboard

Add flag to atmos terraform generate planfile

Open Cerebrovinny opened this issue 4 months ago β€’ 10 comments

##what

  • Add --dir flag to terraform generate planfile command to specify output directory with default naming
  • Make --file and --dir flags mutually exclusive
  • Support both relative and absolute directory paths

##why

  • Support for users who want to organize planfiles in custom directories without manually specifying full file paths
  • Uses default naming convention ({stack}-{component}.planfile.{format}) when directory is specified

references

closes #1403

Summary by CodeRabbit

  • New Features

    • Added a --dir (-d) flag to specify output directory for generated planfiles (absolute or relative); default naming preserved.
    • Enforced mutual exclusivity between --file and --dir and added a clear error when both are provided.
    • Improved path resolution for custom and nested directory scenarios while preserving prior defaults.
  • Tests

    • Added tests for mutual exclusivity and various file/dir path permutations.

Cerebrovinny avatar Oct 10 '25 18:10 Cerebrovinny

[!WARNING]

Rate limit exceeded

@osterman has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 1 seconds before requesting another review.

βŒ› How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 8392b24c5552b576ba7e92b79bf7b326e5c29bda and 045c8420280270dafcec49bfb731bda26537942e.

πŸ“’ Files selected for processing (1)
  • website/blog/2025-12-16-planfile-output-path-flag.mdx (1 hunks)

[!NOTE]

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

πŸ“ Walkthrough

Walkthrough

The changes add a --dir flag to the terraform generate planfile command, allowing users to specify a custom output directory for generated planfiles with automatic naming conventions. The implementation introduces the Dir field to PlanfileOptions, refactors path resolution logic to support three modes (explicit file, custom directory, or default behavior), and enforces mutual exclusivity between --file and --dir flags through validation and tests.

Changes

Cohort / File(s) Summary
Core planfile generation
internal/exec/terraform_generate_planfile.go
Added Dir field to PlanfileOptions struct; updated ExecuteTerraformGeneratePlanfileCmd to read dir flag and validate mutual exclusivity with file flag; refactored resolvePlanfilePath signature to accept PlanfileOptions instead of individual parameters; reworked path resolution into three modes (explicit file, custom directory, or default from config).
Planfile tests
internal/exec/terraform_generate_planfile_test.go
Added test cases for --dir flag functionality, mutual exclusivity validation between --file and --dir, and new TestResolvePlanfilePath to validate path resolution across default, custom file, and directory scenarios; extended test cleanup to handle additional directories.
Command interface
cmd/terraform/generate/planfile.go
Introduced output-path flag with Viper configuration and environment variable (ATMOS_OUTPUT_PATH) support; added mutual exclusivity enforcement between file and output-path flags; extended PlanfileOptions with OutputPath field.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Focus areas: Mutual exclusivity logic in both command and execution paths; resolvePlanfilePath refactoring with three distinct resolution modes; test coverage for all three path resolution scenarios
  • Note: Multiple validation points (CLI flag level and runtime execution) require verification to ensure consistency across entry and execution paths

Possibly related PRs

  • cloudposse/atmos#1214 β€” Extends and modifies the same planfile command implementation with additional Dir/OutputPath fields and mutual-exclusivity checks
  • cloudposse/atmos#1813 β€” Modifies internal/exec/terraform_generate_planfile.go with refactoring that introduces ExecuteGeneratePlanfile and aliasing of PlanfileOptions

Suggested reviewers

  • aknysh

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR implements --dir flag but discussion indicates --output-path aligns better with codebase conventions (--*-path pattern used for path-related flags). Consider renaming --dir to --output-path to match the repository's naming convention for path-related flags and improve consistency with existing patterns.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
βœ… Passed checks (3 passed)
Check name Status Explanation
Description Check βœ… Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check βœ… Passed The title clearly describes the main change: adding a flag to the terraform generate planfile command.
Out of Scope Changes check βœ… Passed All changes directly support the --dir flag feature and mutual exclusivity with --file, staying within scope of issue #1403.

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

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

coderabbitai[bot] avatar Oct 10 '25 18:10 coderabbitai[bot]

Codecov Report

:x: Patch coverage is 88.46154% with 3 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 73.05%. Comparing base (7ef5183) to head (045c842). :warning: Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/terraform/generate/planfile.go 60.00% 2 Missing :warning:
internal/exec/terraform_generate_planfile.go 95.23% 1 Missing :warning:
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1612      +/-   ##
==========================================
+ Coverage   73.03%   73.05%   +0.02%     
==========================================
  Files         606      606              
  Lines       56329    56354      +25     
==========================================
+ Hits        41138    41170      +32     
+ Misses      12280    12277       -3     
+ Partials     2911     2907       -4     
Flag Coverage Ξ”
unittests 73.05% <88.46%> (+0.02%) :arrow_up:

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

Files with missing lines Coverage Ξ”
internal/exec/terraform_generate_planfile.go 61.11% <95.23%> (+5.38%) :arrow_up:
cmd/terraform/generate/planfile.go 32.81% <60.00%> (+2.30%) :arrow_up:

... and 3 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 Oct 10 '25 18:10 codecov[bot]

@coderabbitai can you confirm that the --dir flag would be the most conventional (consistent) name for this flag in this codebase for what it does?

osterman avatar Oct 10 '25 23:10 osterman

[!TIP] For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

echo "=== Searching for directory-related flags in cmd/ directory ==="
rg -n --type=go -C3 'flags\.(String|StringVar).*[Dd]ir' cmd/

echo -e "\n=== Searching for --directory flags ==="
rg -n --type=go -C2 'directory.*flag|flag.*directory' cmd/

echo -e "\n=== Searching for output-related flags ==="
rg -n --type=go -C2 'flags\.(String|StringVar).*output' cmd/

echo -e "\n=== Checking existing flag definitions for patterns ==="
rg -n --type=go 'PersistentFlags\(\)\.(String|StringVar)' cmd/ | head -30

echo -e "\n=== Looking for similar file/path related flags ==="
rg -n --type=go -C2 'flags\.(String|StringVar).*[Ff]ile' cmd/ | head -40

Length of output: 5044


🏁 Script executed:

#!/bin/bash

echo "=== Looking for directory handling in internal/exec ==="
rg -n --type=go -C2 'Dir|Directory' internal/exec/*.go | grep -i 'flag\|param\|arg\|option' | head -40

echo -e "\n=== Checking terraform_generate_varfile patterns ==="
cat cmd/terraform_generate_varfile.go | grep -A10 -B5 "PersistentFlags"

echo -e "\n=== Checking terraform_generate_varfiles patterns ==="
cat cmd/terraform_generate_varfiles.go | grep -A15 -B5 "PersistentFlags"

echo -e "\n=== Looking at how other generate commands handle output paths ==="
rg -n --type=go 'func.*Generate.*file' internal/exec/ | head -20

echo -e "\n=== Checking for any base-path, output-path, or path-related flags ==="
rg -n --type=go 'flags\.String.*path|flags\.String.*base' cmd/ | head -20

Length of output: 7094


🏁 Script executed:

#!/bin/bash

echo "=== Checking varfiles implementation for path handling ==="
rg -n --type=go -A5 -B5 'file-template' internal/exec/terraform_generate_varfiles.go | head -50

echo -e "\n=== Looking for any other output directory patterns in CLI ==="
rg -n --type=go 'output.*dir|output.*path|dest.*dir|destination' cmd/ | head -20

echo -e "\n=== Checking if there are flags with 'dir' anywhere in codebase ==="
rg -n --type=go 'flags\.(String|StringVar|StringP).*"dir"' . | head -20

echo -e "\n=== Checking backend/state file related flags for patterns ==="
rg -n --type=go 'backend|state.*file|state.*path' cmd/terraform_generate*.go | head -20

echo -e "\n=== Looking at cobra flag patterns across all commands ==="
fd -e go . cmd/ -x grep -l 'PersistentFlags' | head -10 | xargs -I {} bash -c 'echo "=== {} ===" && grep -A2 "PersistentFlags.*String" {} | head -20'

Length of output: 9021


@osterman, based on my analysis of the codebase, --dir does not have a precedent in this repository. Here's what I found:

Existing output path patterns:

  • --file (with -f short form) - used extensively for single file outputs (varfile, planfile, workflows)
  • --file-template - used for bulk generation with path templates (varfiles, backends)
  • --output-path - used once in atlantis generate repo-config for specifying output location

Key findings:

  • βœ… --dir is concise and clear in context
  • ❌ No existing --dir or --directory flags in the codebase
  • πŸ€” The closest precedent is --output-path, though it's used for complete paths rather than directory-only

Alternative to consider: --output-dir might be more conventional as it:

  1. Is more explicit about its purpose (output directory)
  2. Is commonly used in CLI tools (terraform, docker, kubectl, etc.)
  3. Follows the pattern of being more descriptive rather than abbreviated
  4. Better distinguishes itself from the existing --file flag

Recommendation: Consider renaming --dir to --output-dir (with -d as the short form) for better clarity and consistency with common CLI conventions, especially given the lack of any existing --dir precedent in the codebase.

Would you like me to search for how similar tools in the ecosystem handle directory output flags?

coderabbitai[bot] avatar Oct 10 '25 23:10 coderabbitai[bot]

[!TIP] For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

echo "=== Searching for directory-related flags in cobra commands ==="
# Search for flag registrations that relate to directories
rg -n -C2 --type=go '(PersistentFlags|Flags)\(\)\.(String|StringVar|StringP|StringVarP)\(' | rg -i '(dir|directory|output|path)' | head -50

echo -e "\n=== Searching for specific directory flag patterns ==="
# Look for common directory flag naming patterns
rg -n --type=go '(--dir|--directory|--output-dir|--out-dir|--path)' cmd/ internal/

echo -e "\n=== Searching for flag help text mentioning directories ==="
# Search for flag descriptions that mention directory or output directory
rg -n --type=go -A1 'PersistentFlags\(\)\.String.*[Pp]\("' cmd/ | rg -i 'director' | head -30

echo -e "\n=== Checking existing terraform commands for similar patterns ==="
# Look at other terraform commands to see naming conventions
fd -e go . cmd/ | xargs rg -l 'terraform.*Cmd' | head -10

Length of output: 8229


🏁 Script executed:

#!/bin/bash

echo "=== Searching for all path-related flags in the codebase ==="
# Get a comprehensive view of all path-related flags
rg -n --type=go 'PersistentFlags\(\)\.(String|StringP|StringSlice).*path' cmd/ | grep -v 'test.go'

echo -e "\n=== Checking generate commands for output specification patterns ==="
# Look at how other generate commands specify output
cat cmd/terraform_generate_varfile.go | grep -A2 -B2 'PersistentFlags'
echo "---"
cat cmd/helmfile_generate_varfile.go | grep -A2 -B2 'PersistentFlags'

echo -e "\n=== Searching for output directory patterns across all commands ==="
# Search for any output-related directory flags
rg -n --type=go '(output|out).*dir' cmd/ internal/

echo -e "\n=== Looking at all flags in terraform_generate_planfile.go ==="
# See the complete context of flags in the new command
cat cmd/terraform_generate_planfile.go

Length of output: 8816


@osterman: After analyzing the codebase, I can confirm that --dir is NOT the most conventional name for this flag. The codebase has a strong and consistent pattern of using the --*-path suffix for directory and path-related flags.

Evidence of the established pattern:

Path-related flags throughout the codebase:

  • --base-path - Base path for Atmos project
  • --config-path - Paths to configuration directories
  • --schema-path - Path to schema file
  • --module-paths - Paths to OPA policy modules
  • --repo-path - Filesystem path to repository
  • --output-path - Output path (used in atlantis generate repo config)

Recommendation:

The most conventional and consistent name would be --output-path (following the pattern used by the atlantis generate repo config command), or alternatively --dir-path to maintain the --*-path suffix convention.

The current --file flag already follows the convention used by other generate commands (terraform generate varfile and helmfile generate varfile), so keeping --file as-is and changing --dir to --output-path would make the command fully consistent with the rest of the codebase.

coderabbitai[bot] avatar Oct 10 '25 23:10 coderabbitai[bot]

@Cerebrovinny see the @coderabbitai comment about flag name.

The most conventional and consistent name would be --output-path

osterman avatar Oct 14 '25 01:10 osterman

[!TIP] For best results, initiate chat on the files or code changes.

@osterman: Acknowledged. I'm here to help if @Cerebrovinny has any questions about the flag renaming or needs assistance with the refactoring to use --output-path instead of --dir.

coderabbitai[bot] avatar Oct 14 '25 01:10 coderabbitai[bot]

[!TIP] For best results, initiate chat on the files or code changes.

@osterman: Thank you for forwarding the recommendation. I'm standing by to assist @Cerebrovinny with any questions about refactoring from --dir to --output-path, or if you need me to help verify the changes once implemented.

coderabbitai[bot] avatar Oct 14 '25 01:10 coderabbitai[bot]

[!WARNING]

Changelog Entry Required

This PR is labeled minor or major but doesn't include a changelog entry.

Action needed: Add a new blog post in website/blog/ to announce this change.

Example filename: website/blog/2025-12-09-feature-name.mdx

Alternatively: If this change doesn't require a changelog entry, remove the minor or major label.

github-actions[bot] avatar Dec 09 '25 20:12 github-actions[bot]

[!WARNING]

Changelog Entry Required

This PR is labeled minor or major but doesn't include a changelog entry.

Action needed: Add a new blog post in website/blog/ to announce this change.

Example filename: website/blog/2025-12-16-feature-name.mdx

Alternatively: If this change doesn't require a changelog entry, remove the minor or major label.

github-actions[bot] avatar Dec 09 '25 20:12 github-actions[bot]

πŸ’₯ This pull request now has conflicts. Could you fix it @Cerebrovinny? πŸ™

mergify[bot] avatar Dec 16 '25 18:12 mergify[bot]

[!WARNING]

Changelog Entry Required

This PR is labeled minor or major but doesn't include a changelog entry.

Action needed: Add a new blog post in website/blog/ to announce this change.

Example filename: website/blog/2025-12-16-feature-name.mdx

Alternatively: If this change doesn't require a changelog entry, remove the minor or major label.

github-actions[bot] avatar Dec 16 '25 23:12 github-actions[bot]

Dependency Review

βœ… No vulnerabilities or license issues found.

Scanned Files

None

github-actions[bot] avatar Dec 17 '25 04:12 github-actions[bot]