jf helm package fails to collect build-info when Chart.yaml and packaged chart archive are in different directories. - #501
jf helm package fails to collect build-info when Chart.yaml and packaged chart archive are in different directories.#501kumadee wants to merge 4 commits into
jf helm package fails to collect build-info when Chart.yaml and packaged chart archive are in different directories.#501Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
naveenku-jfrog
added
the
safe to test
github-actions
Bot
removed
the
safe to test
|
PR's branch is out of sync and all cli integration tests are failing, please fix these first. |
|
I have rebased the PR branch with the default branch. Unit tests are passing on my local. Waiting for the integration tests to be executed on CI. |
|
@naveenku-jfrog Could you please let me know what else do I need to do so that this PR can move forward? Do I need to ask JFrog support to give the reviewers any additional details? |
|
@naveenku-jfrog @bhanurp @udaykb2 Dear reviewers, Could you please let me know what else do I need to do so that this PR can move forward? |
| func isChartDir(dir string) bool { | ||
| chartPath := filepath.Join(dir, flexpack.ChartYaml) | ||
| _, err := os.Stat(chartPath) | ||
| return !errors.Is(err, os.ErrNotExist) |
There was a problem hiding this comment.
This treats any non-ErrNotExist error (permission denied, I/O error, etc.) as “is a chart dir,” which can send FlexPack down a bad path. Prefer existence success only:
_, err := os.Stat(chartPath)
return err == nil
Then errors can be dropped.
There was a problem hiding this comment.
Agreed. Updated as suggested. Added corresponding unit tests.
| removeDuplicateDependencies(buildInfoOld) | ||
| err = saveBuildInfo(buildInfoOld, buildName, buildNumber, project) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to save build info") |
There was a problem hiding this comment.
fmt.Errorf("failed to save build info") still drops the underlying error; should use %w.
| name: "chart path with destination flag", | ||
| args: func(chartPath, destination string) []string { | ||
| return []string{chartPath, "--destination", destination} | ||
| }, |
There was a problem hiding this comment.
Bug report used short -d. Add a case:
return []string{chartPath, "-d", destination}
Missing cases:
No chart among paths → expect "no valid Helm chart directory found...".
Direct unit tests for isChartDir (exists / missing / non-dir path).
Optional: destination dir that exists but has no Chart.yaml (regression for the ticket).
There was a problem hiding this comment.
Added the test case as per bug report. Added missing test cases.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesHelm packaging now detects one directory containing Helm package flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLIArguments
participant handlePackageCommand
participant isChartDir
participant BuildInfoCollection
participant BuildInfo
CLIArguments->>handlePackageCommand: provide chart and option paths
handlePackageCommand->>isChartDir: resolve and inspect paths
isChartDir-->>handlePackageCommand: return selected chart directory
handlePackageCommand->>BuildInfoCollection: collect build info once
BuildInfoCollection-->>handlePackageCommand: return module and dependencies
handlePackageCommand->>BuildInfo: update and save build info
BuildInfo-->>handlePackageCommand: return result or wrapped error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
artifactory/commands/helm/package_test.go (1)
289-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert dependency uniqueness.
The map hides duplicate dependency IDs, so this helper passes even if duplicate removal regresses.
Proposed test assertion
for _, dep := range module.Dependencies { depIds[dep.Id] = true require.NotEmpty(t, dep.Sha256) } + require.Len(t, depIds, len(module.Dependencies), "Expected unique dependencies") require.True(t, depIds["subchart-a:0.1.0"])🤖 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 `@artifactory/commands/helm/package_test.go` around lines 289 - 295, Update the dependency assertions in the package test to verify uniqueness explicitly rather than relying only on the depIds map. Assert that the number of collected dependency IDs matches the expected unique dependency count, while retaining the existing checks for subchart-a and subchart-b.
🤖 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.
Nitpick comments:
In `@artifactory/commands/helm/package_test.go`:
- Around line 289-295: Update the dependency assertions in the package test to
verify uniqueness explicitly rather than relying only on the depIds map. Assert
that the number of collected dependency IDs matches the expected unique
dependency count, while retaining the existing checks for subchart-a and
subchart-b.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d6b5b2bf-25af-420d-a6b7-7a65719f1aef
📒 Files selected for processing (2)
artifactory/commands/helm/package.goartifactory/commands/helm/package_test.go
…n helm chart archive When `helm package /path/to/chart --destination /tmp/target` is executed, we need to filter out directories which doesn't contain Chart.yaml.
|
@naveenku-jfrog Could you please re-review the changes? |
2 participants
Original issue was raised as a part of Jfrog support ticket 426969.
Here is the problem statement:
When
jf helm packagecommand is executed with multiple options contains different paths e.g. chart directory path, chart archive destination path, keyring path and all of these path are different, then the command fails as it expects a Chart.yaml in each of these directories.In the below log the Chart.yaml is in
./target/my-app-chartand themy-app-384-SNAPSHOT.tgzis successfully created in./target/helm. For some reason, the jf cli expects the Chart.yaml to be always in the same directory as the Chart.yaml.Proposed solution: Filter out the different paths given in the
helm package CHART_DIR ....command. There can be only 1 chart directory in the command, which can be checked by the presence ofChart.yaml.Summary by CodeRabbit
Chart.yaml, while ignoring unrelated paths.