fix(PL-6888): support concurrent chart pulls#23
Conversation
📝 WalkthroughWalkthroughReleaseGK now uses a cache-aware ChangesChart pulling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ReleaseGK
participant ChartPuller
participant HelmCLI
participant CacheDirectory
participant Logger
ReleaseGK->>ChartPuller: Pull helm.PullOptions
ChartPuller->>CacheDirectory: Check output directory
CacheDirectory-->>ChartPuller: Existing non-empty cache or status
ChartPuller->>HelmCLI: Pull chart when cache is missing or empty
HelmCLI-->>ChartPuller: Output or pull error
ChartPuller->>Logger: Log successful pull
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
cmd/operator/puller.go (1)
40-46: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider validating cache contents beyond non-empty directory check.
The current cache hit logic only checks
len(entries) > 0. A directory with a stray.DS_Storeor partial file would be treated as a valid cache. Consider checking for expected chart files (e.g.,Chart.yaml) for a more robust cache validation.🤖 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 `@cmd/operator/puller.go` around lines 40 - 46, The cache-hit logic around os.ReadDir should validate that the output directory contains the expected chart artifact, such as Chart.yaml, rather than treating any non-empty directory as valid. Update the condition to require the expected file while preserving the existing missing-directory and read-error handling.
🤖 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 `@cmd/operator/puller.go`:
- Around line 40-50: Update the cli.Pull error path to remove opts.OutputDir
before returning the wrapped pull error, ensuring partially written chart data
cannot be reused on subsequent attempts. Preserve the existing error message and
handle cleanup without masking the original pull failure.
- Around line 34-35: Handle the error returned by opts.Chart.ToURL() before
using the URL in the puller flow. In the code around the puller mutex setup,
return or propagate the error when conversion fails so url.String() is only
called for a valid URL; apply the same guard to the later URL usage as needed.
---
Nitpick comments:
In `@cmd/operator/puller.go`:
- Around line 40-46: The cache-hit logic around os.ReadDir should validate that
the output directory contains the expected chart artifact, such as Chart.yaml,
rather than treating any non-empty directory as valid. Update the condition to
require the expected file while preserving the existing missing-directory and
read-error handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4087014d-029a-42d5-bf61-b469dda82958
📒 Files selected for processing (3)
cmd/operator/main.gocmd/operator/puller.gogo.mod
| url, _ := opts.Chart.ToURL() | ||
| mutex, _ := puller.Mutexes.LoadOrStore(url.String(), new(sync.Mutex)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Handle the ToURL() error to prevent nil pointer dereference.
The error from opts.Chart.ToURL() is discarded. If it returns a nil URL with a non-nil error, url.String() on lines 35 and 52 will panic. This is a reachable runtime crash on any malformed chart reference.
🐛 Proposed fix
- url, _ := opts.Chart.ToURL()
+ url, err := opts.Chart.ToURL()
+ if err != nil {
+ return fmt.Errorf("failed to resolve chart URL: %w", err)
+ }
mutex, _ := puller.Mutexes.LoadOrStore(url.String(), new(sync.Mutex))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| url, _ := opts.Chart.ToURL() | |
| mutex, _ := puller.Mutexes.LoadOrStore(url.String(), new(sync.Mutex)) | |
| url, err := opts.Chart.ToURL() | |
| if err != nil { | |
| return fmt.Errorf("failed to resolve chart URL: %w", err) | |
| } | |
| mutex, _ := puller.Mutexes.LoadOrStore(url.String(), new(sync.Mutex)) |
🤖 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 `@cmd/operator/puller.go` around lines 34 - 35, Handle the error returned by
opts.Chart.ToURL() before using the URL in the puller flow. In the code around
the puller mutex setup, return or propagate the error when conversion fails so
url.String() is only called for a valid URL; apply the same guard to the later
URL usage as needed.
| if entries, err := os.ReadDir(opts.OutputDir); err == nil && len(entries) > 0 { | ||
| // If the output directory exists and has content in it, | ||
| // then it has been pulled by another goroutine: no need to pull the chart | ||
| return nil | ||
| } else if err != nil && !errors.Is(err, os.ErrNotExist) { | ||
| return fmt.Errorf("failed to stat chart cache: %w", err) | ||
| } | ||
|
|
||
| if err := cli.Pull(ctx, opts); err != nil { | ||
| return fmt.Errorf("%w: %q", err, &buffer) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clean up output directory on pull failure to prevent cache poisoning.
If cli.Pull fails after partially writing files to opts.OutputDir, the next attempt will see a non-empty directory and skip the pull, silently using incomplete or corrupted chart data. Consider removing the directory contents (or the directory itself) when the pull returns an error.
🛡️ Proposed fix: clean up on failure
if err := cli.Pull(ctx, opts); err != nil {
+ os.RemoveAll(opts.OutputDir)
return fmt.Errorf("%w: %q", err, &buffer)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if entries, err := os.ReadDir(opts.OutputDir); err == nil && len(entries) > 0 { | |
| // If the output directory exists and has content in it, | |
| // then it has been pulled by another goroutine: no need to pull the chart | |
| return nil | |
| } else if err != nil && !errors.Is(err, os.ErrNotExist) { | |
| return fmt.Errorf("failed to stat chart cache: %w", err) | |
| } | |
| if err := cli.Pull(ctx, opts); err != nil { | |
| return fmt.Errorf("%w: %q", err, &buffer) | |
| } | |
| if entries, err := os.ReadDir(opts.OutputDir); err == nil && len(entries) > 0 { | |
| // If the output directory exists and has content in it, | |
| // then it has been pulled by another goroutine: no need to pull the chart | |
| return nil | |
| } else if err != nil && !errors.Is(err, os.ErrNotExist) { | |
| return fmt.Errorf("failed to stat chart cache: %w", err) | |
| } | |
| if err := cli.Pull(ctx, opts); err != nil { | |
| os.RemoveAll(opts.OutputDir) | |
| return fmt.Errorf("%w: %q", err, &buffer) | |
| } |
🤖 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 `@cmd/operator/puller.go` around lines 40 - 50, Update the cli.Pull error path
to remove opts.OutputDir before returning the wrapped pull error, ensuring
partially written chart data cannot be reused on subsequent attempts. Preserve
the existing error message and handle cleanup without masking the original pull
failure.
2 participants
Summary by CodeRabbit