refactor(deployer): unify deployer interface across helm, argocd, argocd-helm (#571)

[lockwobr]

Summary

Migrate all three deployers (helm, argocd, argocd-helm) to the deployer.Deployer interface and unify bundler routing through a single Generate(ctx, outputDir) contract.

Motivation / Context

PR #527 introduced deployer.Deployer and deployer.Output but only argocdhelm implemented the interface. The helm and argocd deployers still used their own GeneratorInput/GeneratorOutput types and were called via concrete methods in bundler.go. This meant deployers weren't mockable, each had redundant output types, and the shared/ sub-package added unnecessary indirection.

Fixes: #571
Related: #527, #515

Fixes:
Related:

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Refactoring (no functional changes)
• Build/CI/tooling

Component(s) Affected

• CLI (cmd/aicr, pkg/cli)
• API server (cmd/aicrd, pkg/api, pkg/server)
• Recipe engine / data (pkg/recipe)
• Bundlers (pkg/bundler, pkg/component/*)
• Collectors / snapshotter (pkg/collector, pkg/snapshotter)
• Validator (pkg/validator)
• Core libraries (pkg/errors, pkg/k8s)
• Docs/examples (docs/, examples/)
• Other: ____________

Implementation Notes

• Consolidated deployer/shared/ utilities into deployer/ package
• Absorbed GeneratorInput fields into Generator structs for helm and argocd ("Generator IS the input" pattern matching argocdhelm)
• Deleted GeneratorInput, GeneratorOutput, and NewGenerator from helm and argocd packages
• Replaced three makeHelmBundle/makeArgoCD/makeArgoCDHelmChart methods with buildDeployer (factory) + runDeployer (common post-generation)
• Removed HasDynamicValues() from the interface — it was never called through the interface; dynamic value checks use bundler.Config directly
• The interface is now a single method: Generate(ctx, outputDir) (*Output, error)
• The external Make() API is unchanged

Testing

# Commands run (prefer `make qualify` for non-trivial changes)
make qualify

Risk Assessment

• Low — Isolated change, well-tested, easy to revert
• Medium — Touches multiple components or has broader impact
• High — Breaking change, affects critical paths, or complex rollout

Rollout notes:

Checklist

• Tests pass locally (make test with -race)
• Linter passes (make lint)
• I did not skip/disable tests to make CI green
• I added/updated tests for new functionality
• I updated docs if user-facing behavior changed
• Changes follow existing patterns in the codebase
• Commits are cryptographically signed (git commit -S) — GPG signing info

Summary by CodeRabbit

Release Notes

• Refactor
  • Streamlined internal deployer implementation for improved maintainability and consistency across bundling processes.
  • Consolidated bundler logic to enhance how external data files and deployment configurations are handled during bundle creation.
  • Reorganized helper utilities for better code organization and consistency.

[github-actions]

Coverage Report ✅

Metric	Value
Coverage	75.3%
Threshold	70%
Status	Pass

Coverage Badge

![Coverage](https://img.shields.io/badge/coverage-75.3%25-green)

Merging this branch changes the coverage (2 decrease, 3 increase)

Impacted Packages	Coverage Δ	🤖
github.com/NVIDIA/aicr/pkg/bundler	65.00% (+3.20%)	👍
github.com/NVIDIA/aicr/pkg/bundler/deployer	91.78% (+91.78%)	🌟
github.com/NVIDIA/aicr/pkg/bundler/deployer/argocd	86.49% (-0.18%)	👎
github.com/NVIDIA/aicr/pkg/bundler/deployer/argocdhelm	71.65% (+0.16%)	👍
github.com/NVIDIA/aicr/pkg/bundler/deployer/helm	86.91% (-0.07%)	👎

---

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/NVIDIA/aicr/pkg/bundler/bundler.go	62.29% (+4.09%)	358 (-20)	223 (+3)	135 (-23)	👍
github.com/NVIDIA/aicr/pkg/bundler/deployer/argocd/argocd.go	86.49% (-0.18%)	74 (-1)	64 (-1)	10	👎
github.com/NVIDIA/aicr/pkg/bundler/deployer/argocd/doc.go	0.00% (ø)	0	0	0
github.com/NVIDIA/aicr/pkg/bundler/deployer/argocdhelm/argocdhelm.go	71.65% (+0.16%)	261 (-2)	187 (-1)	74 (-1)	👍
github.com/NVIDIA/aicr/pkg/bundler/deployer/deployer.go	0.00% (ø)	0	0	0
github.com/NVIDIA/aicr/pkg/bundler/deployer/helm/doc.go	0.00% (ø)	0	0	0
github.com/NVIDIA/aicr/pkg/bundler/deployer/helm/helm.go	86.91% (-0.07%)	191 (-1)	166 (-1)	25	👎
github.com/NVIDIA/aicr/pkg/bundler/deployer/helpers.go	91.78% (+91.78%)	73 (+73)	67 (+67)	6 (+6)	🌟

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

[yuanchen8911]

#	File	Line	Severity	Description
1	pkg/bundler/bundler.go	329, 379	critical	Double copyDataFiles call for helm deployer — buildDeployer copies data files into dir/data/, then runDeployer calls copyDataFiles again. os.CopyFS fails with fs.ErrExist when files already exist.
2	pkg/bundler/bundler.go	367-371	major	writeRecipeFile now runs for argocd and argocd-helm deployers (previously helm-only). Adds an undocumented recipe.yaml to those bundle outputs and changes TotalFiles/TotalSize metrics.

Open Questions

• The comment at line 375 says "copyDataFiles returns the already-copied list here" — but copyDataFiles doesn't detect already-copied files; it unconditionally calls os.CopyFS. Was the intent to make it idempotent, or to skip the second call for helm?
• No test coverage for --data flag with any deployer in the changed test files.

[yuanchen8911]

#575 (comment)

[ArangoGutierrez]

The unification is a net improvement (-189 lines). The buildDeployer / runDeployer split is clean. Two things to address:

1. getDataFileList vs copyDataFiles split -- In the Helm path, buildDeployer passes getDataFileList() to the Generator for checksum tracking, and runDeployer calls copyDataFiles() for the actual filesystem copy + attestation list. This is functionally correct but the two functions compute the same list independently. If they ever diverge, the Helm generator would reference files that attestation does not. Consider having copyDataFiles return the list and pass it to the Generator, or document the coupling.

2. Reviewer questions about deleted methods -- @ayuskauskas asked about the removed DeploymentInfo() in deployer.go and argocdhelm.go. lockwobr confirmed they were unused. That checks out -- deployment info is now built in runDeployer from deployer.Output fields directly.

Ayuskauskas's questions are resolved. The getDataFileList/copyDataFiles coupling is the remaining design point.

[coderabbitai]

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

All three deployers (helm, argocd, argocd-helm) migrated to a unified deployer.Deployer interface pattern. Generators now use a "configured struct" model where configuration is set on fields before calling Generate(ctx, outputDir), eliminating separate GeneratorInput/GeneratorOutput types. The deployer/shared package was renamed to deployer. Bundler refactored to use generic deployer interface methods instead of type-specific implementations.

Changes

Cohort / File(s)	Summary
Bundler Core pkg/bundler/bundler.go	Refactored DefaultBundler.Make to replace deployer-specific switch statements with buildDeployer() and runDeployer() functions. Early copying of external --data files via copyDataFiles(). Consolidated bundle result construction with unified file/size accounting and conditional recipe.yaml writing for Helm deployer only.
Deployer Interface & Utilities pkg/bundler/deployer/deployer.go, pkg/bundler/deployer/helpers.go, pkg/bundler/deployer/helpers_test.go	Removed HasDynamicValues() method from Deployer interface. Renamed shared package to deployer. Updated documentation to reflect all three deployers implementing the interface.
Helm Deployer Migration pkg/bundler/deployer/helm/helm.go, pkg/bundler/deployer/helm/helm_test.go, pkg/bundler/deployer/helm/doc.go	Removed GeneratorInput/GeneratorOutput types and NewGenerator() constructor. Changed Generate signature to accept only ctx and outputDir. Migrated helper function calls from shared to deployer package. Updated tests to directly instantiate configured Generator struct.
ArgoCD Deployer Migration pkg/bundler/deployer/argocd/argocd.go, pkg/bundler/deployer/argocd/argocd_test.go, pkg/bundler/deployer/argocd/doc.go	Removed GeneratorInput/GeneratorOutput types and NewGenerator() constructor. Changed Generate signature from (ctx, input, outputDir) to (ctx, outputDir). Updated all field references from input.* to g.*. Migrated helper calls to deployer package. Added compile-time interface assertion.
ArgoCD-Helm Deployer Update pkg/bundler/deployer/argocdhelm/argocdhelm.go	Removed HasDynamicValues() method. Updated argocd generator instantiation to configured struct pattern. Migrated helper function calls from shared to deployer package.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 Three deployers, once apart,
Now unified by interface art!
Generators configured, clean and bright,
The bundler's refactored—what a sight! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR substantially meets the five core objectives from issue #571: migrating helm/argocd to deployer.Deployer, removing GeneratorInput/GeneratorOutput, refactoring bundler routing, and consolidating shared utilities. However, a critical regression introduces double-copy errors for helm --data files and unwanted recipe.yaml output for argocd/argocd-helm deployers.	Fix the helm deployer double-copy bug (buildDeployer and runDeployer both call copyDataFiles), and revert recipe.yaml writing to helm-only to prevent argocd/argocd-helm regressions and unexpected TotalFiles/TotalSize changes.
Out of Scope Changes check	⚠️ Warning	All code changes are directly in scope; however, the implementation introduces unintended behavioral regressions (helm double-copy, argocd/argocd-helm recipe.yaml generation) that violate the stated objective of unified interface without breaking changes.	Review and fix the double-copy mechanism in bundler.go and ensure recipe.yaml is written only for Helm deployer to avoid breaking argocd and argocd-helm bundle outputs.
Docstring Coverage	⚠️ Warning	Docstring coverage is 18.97% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately and concisely describes the main refactoring objective: unifying the deployer interface across the three deployer implementations (helm, argocd, argocd-helm).

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/deployer-interface

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[yuanchen8911]

/lgtm

Re-checked the current PR head and the two originally flagged issues are addressed:

• Helm no longer re-copies external --data files after Generate(), so the file-exists regression is gone.
• recipe.yaml is now Helm-only, so ArgoCD and argocd-helm no longer add a post-checksum unchecked recipe file.

There is still a pre-existing ArgoCD external-data checksum gap, but it is not introduced by this PR and should be tracked separately as follow-up work.

[yuanchen8911]

Follow-up detail on the pre-existing ArgoCD external-data issue:

With --deployer argocd, external --data files are copied into the bundle before generation, but the ArgoCD generator still computes checksums.txt from output.Files only and does not receive/include the copied data/... file list. That means the end state is unchanged from before this PR: for ArgoCD bundles created with checksums enabled, external data files are still outside the verified checksum set.

This is not introduced by #575; the old flow also copied ArgoCD data files after Generate(), so they were already outside checksums.txt. The refactor improves Helm and removes the ArgoCD/argocd-helm recipe.yaml regression, but this ArgoCD --data integrity gap remains pre-existing follow-up work.

Suggested follow-up: add DataFiles []string to argocd.Generator and append them to the generator output before the checksum call, mirroring the Helm generator pattern.