RHTAP-6522: Add ChartFS.ExtractTo Method#58
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA new public method ChangesChart Extraction Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/chartfs/chartfs.go (1)
149-152: ⚡ Quick winAdd chart directory context to extraction failures
When one chart fails, returning the raw error makes triage slower in multi-chart extraction flows. Wrapping with
chartDirhere will keep errors actionable.Suggested fix
for _, chartDir := range chartDirs { if err := c.extractChartDir(destDir, chartDir); err != nil { - return err + return fmt.Errorf("extracting chart %q: %w", chartDir, err) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/chartfs/chartfs.go` around lines 149 - 152, The loop over chartDirs returns raw errors from extractChartDir which loses which chart failed; update the loop in the function containing chartDirs to wrap/annotate the returned error with the chartDir value (e.g., return fmt.Errorf("extractChartDir %s: %w", chartDir, err) or use errors.Wrapf) so callers can see which chart directory caused the failure; target the call to extractChartDir in the same loop to add this context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/chartfs/chartfs.go`:
- Around line 110-113: The code uses os.MkdirAll and os.OpenFile but doesn't
enforce the desired permission bits on pre-existing paths; update the extraction
logic around d.IsDir()/target and the file-write path (the block that uses
os.OpenFile and writes files) to explicitly set permissions after creation by
calling os.Chmod(target, 0o755) for directories and os.Chmod(target, 0o644) for
files (and do so unconditionally after creation/write so existing entries are
normalized); ensure you perform the Chmod even when MkdirAll or OpenFile reports
no error and apply the same change pattern in the other similar block referenced
(the file-write block that currently opens/writes files) so both directories and
files always end up with the intended modes.
---
Nitpick comments:
In `@internal/chartfs/chartfs.go`:
- Around line 149-152: The loop over chartDirs returns raw errors from
extractChartDir which loses which chart failed; update the loop in the function
containing chartDirs to wrap/annotate the returned error with the chartDir value
(e.g., return fmt.Errorf("extractChartDir %s: %w", chartDir, err) or use
errors.Wrapf) so callers can see which chart directory caused the failure;
target the call to extractChartDir in the same loop to add this context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bbbd470-f0dd-4431-8f66-189233e066a9
📒 Files selected for processing (2)
internal/chartfs/chartfs.gointernal/chartfs/chartfs_test.go
ArgoCD's Helm source type requires an actual directory with `Chart.yaml` and templates on disk, but `ChartFS` only provides in-memory `fs.FS` access. The method `ExtractTo` bridges this gap by discovering chart directories via the existing `walkAndFindChartDirs` and writing their contents to a destination directory, filtering out root-level files and non-chart directories like `_common/`. Assisted-by: Claude
2 participants
ChartFScurrently provides read-only in-memory access to Helm charts viafs.FS, which is sufficient for Helmet's own deployer but not for consumers that need charts materialized on disk. ArgoCD's Helm source type is one such consumer, it expects a real directory containingChart.yamland templates to feed intohelm templateorhelm install.This PR adds
ChartFS.ExtractTo(destDir string) error, which discovers all chart directories (those containing Chart.yaml) in the embedded filesystem and writes them to the given destination path. Root-level files likevalues.yaml.tplandconfig.yamlare intentionally excluded, as are non-chart directories such as _common/. File permissions follow the existing convention:0755for directories,0644for files.The implementation reuses the existing
walkAndFindChartDirsfor chart discovery and follows the file-extraction pattern established ininternal/subcmd/installer.go. Two private helpers —extractChartDirandextractFile— keep the logic decomposed and testable.Summary by CodeRabbit
New Features
Tests