feat: Packer directory-based template support and improvements#1982
Merged
feat: Packer directory-based template support and improvements#1982
Conversation
|
Warning Release Documentation RequiredThis PR is labeled
|
1 similar comment
|
Warning Release Documentation RequiredThis PR is labeled
|
|
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 |
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR introduces directory-based template support for Packer in Atmos by defaulting the template parameter to "." when unspecified, enabling automatic loading of all Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
Member
Author
|
@coderabbitai full review please |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughWalkthroughThis PR adds directory-based template support for Packer in Atmos. When no template is specified, Atmos defaults to the current directory (".") to enable Packer to load all *.pkr.hcl files from a component directory, fixing the issue where variables.pkr.hcl was not being utilized. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
🤖 Fix all issues with AI agents
In `@cmd/packer_build_test.go`:
- Around line 20-71: TestPackerBuildCmd (and
TestPackerBuildCmdWithDirectoryTemplate) only capture stdout and don't
initialize plugins, so stderr plugin errors like "Missing plugins… run packer
init" are missed; before calling Execute() run "packer init" for the same
workDir and skip the test if init fails, redirect/capture both stdout and stderr
(duplicate os.Stderr into the same pipe used for stdout or create a combined
reader) so plugin messages are visible, close the write end after both init and
build, read the combined output into output string, and explicitly check output
for missing-plugin phrases ("Missing plugins", "run packer init") and treat that
case as expected/skip rather than a test failure; update logic in
TestPackerBuildCmd and TestPackerBuildCmdWithDirectoryTemplate accordingly.
In `@internal/exec/packer_output_test.go`:
- Around line 72-85: The test writes YAML with a double-quoted base_path using
tempDir which on Windows contains backslashes that become invalid YAML escapes;
update the os.WriteFile call that writes the config (the fmt.Sprintf block
around base_path) inside packer_output_test.go to either wrap the inserted path
in single quotes or normalize the path (e.g., replace backslashes with forward
slashes) before formatting; ensure you update the variable used in the
fmt.Sprintf (tempDir) so the produced YAML is safe on Windows and the subsequent
parsing in the test succeeds.
In `@internal/exec/packer_test.go`:
- Around line 18-48: captureStdout leaves the global logger pointed at a closed
pipe and can leak file descriptors on panic; update captureStdout to save the
original logger writer (e.g. oldLog := log.Writer()) and restore it in a defer
alongside restoring os.Stdout, and close both pipe ends (w.Close() and
r.Close()) in that defer so the writer is closed even on panic; ensure you still
close w before copying from r when normal execution reaches the read, but rely
on the deferred closes for panic-safe cleanup so the function (and symbols
captureStdout, oldStdout, log.SetOutput, os.Pipe, r, w) always restores state
and frees FDs.
- Around line 230-258: The test TestExecutePacker_Fmt currently ignores the
returned error from ExecutePacker making it ineffective; update the test to
assert the call succeeds by replacing the unused assignment with an assertion
such as assert.NoError(t, err) after calling ExecutePacker(&info, &packerFlags)
(keep the function name ExecutePacker and test name TestExecutePacker_Fmt to
locate the change).
In `@tests/fixtures/scenarios/packer/components/packer/aws/multi-file/README.md`:
- Around line 7-34: In README.md update the top fenced example to include a
language tag (change ``` to ```text) and replace the two bare URLs under
"Related" with markdown links: change the GitHub Issue URL to
[cloudposse/atmos#1937](https://github.com/cloudposse/atmos/issues/1937) and the
Packer docs URL to [HCL
Templates](https://developer.hashicorp.com/packer/docs/templates/hcl_templates);
locate the fenced block and the Related section in aws/multi-file/README.md and
apply these exact substitutions.
🧹 Nitpick comments (2)
website/docs/cli/commands/packer/troubleshooting.mdx (1)
9-9: Unused import.The
Terminalcomponent is imported but not used in this file. Consider removing it.🧹 Suggested cleanup
--- import Terminal from '@site/src/components/Terminal' - +Or simply remove line 9 entirely if the Terminal component isn't needed.
docs/prd/packer-directory-template-support.md (1)
170-178: Add a language tag to the fenced test-output block (non-blocking).markdownlint MD040 flags the unlabeled fence; consider
textorconsolein a doc-only cleanup PR. Based on learnings, markdownlint cleanups indocs/prd/should be handled in a dedicated follow-up.
tests/fixtures/scenarios/packer/components/packer/aws/multi-file/README.md
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/exec/packer_output_test.go`:
- Around line 72-85: The YAML config write in the os.WriteFile call uses tempDir
directly which on Windows contains backslashes that YAML treats as escape
sequences; change the value passed into fmt.Sprintf to filepath.ToSlash(tempDir)
(normalize tempDir to forward slashes) before embedding it into the YAML string,
and add the necessary import for "path/filepath" so the os.WriteFile(configPath,
[]byte(fmt.Sprintf(..., filepath.ToSlash(tempDir))), 0o644) uses the normalized
path.
In `@tests/fixtures/scenarios/packer/components/packer/aws/multi-file/README.md`:
- Around line 7-34: The README.md uses unmarked code fences and bare URLs which
trigger markdownlint (MD040/MD034); update the code fences to include a language
(e.g., change the top triple backticks to ```text and keep the bash block as
```bash) and replace bare URLs in the "Related" list with Markdown link syntax
(e.g., convert the plain https://... links to
[cloudposse/atmos#1937](https://github.com/cloudposse/atmos/issues/1937) and
[HCL
Templates](https://developer.hashicorp.com/packer/docs/templates/hcl_templates));
edit the README.md content around the directory tree code block, the bash
example block, and the Related list to apply these changes.
In `@website/docs/cli/commands/packer/troubleshooting.mdx`:
- Around line 1-279: The page titled "Packer Troubleshooting" is missing
required CLI docs structure; update the MDX by adding the required frontmatter
(title, sidebar_* fields as used elsewhere), an Intro section under the main
header, a Screengrab component usage, a Usage section showing command
invocation, an Arguments/Flags description using a <dl> block, and an Examples
section with one or more runnable examples (or alternatively move this content
out of the cli/commands tree into a general troubleshooting docs folder); ensure
the new sections use the same headings/structures as other files in cli/commands
so the docs build passes.
🧹 Nitpick comments (3)
cmd/packer_build_test.go (1)
30-32: os.Pipe() error is silently ignored.If pipe creation fails, subsequent operations will have undefined behavior. Consider checking the error.
Suggested fix
oldStd := os.Stdout r, w, _ := os.Pipe() + if r == nil || w == nil { + t.Fatal("failed to create pipe for stdout capture") + } os.Stdout = winternal/exec/packer_test.go (2)
253-258: Consider verifying command execution succeeded.The test currently ignores the error entirely. While the comment explains the intent, you could at least log the error for debugging purposes when it does occur.
Suggested improvement
// packer fmt -check returns 0 if files are formatted, non-zero otherwise. // We just verify the command executes without errors. err := ExecutePacker(&info, &packerFlags) - // The test passes regardless of fmt exit code since we're testing command execution. - _ = err + // Log the error for debugging, but don't fail - we're testing command execution. + if err != nil { + t.Logf("packer fmt returned: %v (this may be expected)", err) + }
774-787: Minor:errorSubstringfield unused whenshouldError: false.For test cases where
shouldError: false, theerrorSubstringfield is set but only used in theNotContainsassertion (line 833). The current value works, but it's semantically clearer to leave it empty for non-error cases or rename the field to clarify its dual purpose.
tests/fixtures/scenarios/packer/components/packer/aws/multi-file/README.md
Outdated
Show resolved
Hide resolved
- Remove unused Terminal import from troubleshooting.mdx - Add language tag to PRD code block - Improve packer build tests (run init first, capture stderr, skip if plugins missing) - Fix Windows path quoting in packer_output_test.go (use single quotes) - Improve captureStdout helper (restore logger output, close pipes properly, handle panics) - Add fallback for network timeout in fetch-latest-release plugin - Fix broken documentation link (packer-build → build) - Refactor nested if blocks to reduce complexity (nestif linter) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1982 +/- ##
==========================================
+ Coverage 74.72% 74.75% +0.03%
==========================================
Files 777 777
Lines 71171 71176 +5
==========================================
+ Hits 53180 53210 +30
+ Misses 14525 14507 -18
+ Partials 3466 3459 -7
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
osterman
reviewed
Jan 17, 2026
osterman
reviewed
Jan 17, 2026
osterman
reviewed
Jan 17, 2026
osterman
reviewed
Jan 17, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/packer.go (1)
47-61: Move config validation below theversionguard to allowatmos packer versionwithout base_path.
checkPackerConfigrequiresComponents.Packer.BasePathand runs before theversionsubcommand check. This meansatmos packer versionfails if base_path isn't configured, even though the version command doesn't need component or stack information. Terraform handles this by deferring deeper validation until after the version check. Move the validation below line 61 to match that pattern.Proposed adjustment
- // Validate packer configuration. - if err := checkPackerConfig(&atmosConfig); err != nil { - return err - } - // Add the `command` from `components.packer.command` from `atmos.yaml`. if info.Command == "" { if atmosConfig.Components.Packer.Command != "" { info.Command = atmosConfig.Components.Packer.Command } else { info.Command = cfg.PackerComponentType } } if info.SubCommand == "version" { return ExecuteShellCommand( atmosConfig, info.Command, []string{info.SubCommand}, "", nil, false, info.RedirectStdErr, ) } + + // Validate packer configuration. + if err := checkPackerConfig(&atmosConfig); err != nil { + return err + }
osterman
approved these changes
Jan 17, 2026
|
These changes were released in v1.204.1-rc.2. |
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
2 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
.(current directory) when not specified, allowing Packer to load all*.pkr.hclfiles from the component directory automatically. This aligns with HashiCorp's recommended multi-file configuration patterns.checkPackerConfig()validation to bothExecutePackerandExecutePackerOutputfunctions, ensuring Packer base path is configured before execution.packer buildon abstract (metadata.type: abstract) and locked (metadata.locked: true) components while allowing read-only commands likevalidateandinspect.ErrMissingPackerBasePatherror: Added dedicated error type for missing Packer configuration.PackerFlagsstruct and its fields.TestExecutePacker_Fmtfor the fmt commandTestExecutePacker_ComponentMetadatafor abstract/locked component handlingTestCheckPackerConfigfor configuration validationcaptureStdouttest helper to reduce code duplicationExecutePackerOutputwhy
references
Summary of Changes
New Features
.when no template specified)Bug Fixes
Improvements
packer buildTest Plan
Summary by CodeRabbit
Release Notes
New Features
*.pkr.hclfiles from the component directory without requiring explicit template configuration.--templateflag orsettings.packer.templateto override.Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.