fix: address review issues in preBuildTransitiveSubchartDeps

- Add per-chartDir mutex via getNamedRWMutex to prevent concurrent
  releases referencing the same local chart from racing on helm dep
  build writes (same pattern as forcedDownloadChart for #768).
- Log non-lock errors at Warnf instead of Debugf so users can diagnose
  why strategicMergePatches fail with "no resource matches".
- Document why --skip-refresh is kept (file://-only deps are the common
  case; chartify runs dep build without --skip-refresh afterwards and
  catches stale repos for mixed-dep subcharts).
- Use strings.HasPrefix instead of manual slicing in test helper.

Signed-off-by: yxxhero <aiopsclub@163.com>
This commit is contained in:
yxxhero
2026-07-02 06:37:50 +08:00
committed by yxxhero
parent 08504ad6f6
commit d3cfe04d47
2 changed files with 27 additions and 5 deletions
+2 -1
View File
@@ -3,6 +3,7 @@ package state
import (
"os"
"path/filepath"
"strings"
"testing"
"time"
@@ -121,7 +122,7 @@ func TestPreBuildTransitiveSubchartDeps_VisitsAllTransitiveDeps(t *testing.T) {
}
func hasNestedTgzPrefix(name string) bool {
return len(name) > len("nested-") && name[:7] == "nested-"
return strings.HasPrefix(name, "nested-")
}
// TestPreBuildTransitiveSubchartDeps_HandlesMissingChartYaml verifies the
+25 -4
View File
@@ -1927,15 +1927,32 @@ func (st *HelmState) preBuildSubchartDepsRecursive(chartDir string, visited map[
// runHelmDependencyBuild runs `helm dependency build` on chartDir, falling back
// to `helm dependency update` if the lock file is missing or out of sync. It
// uses --skip-refresh to avoid hitting the network for local file:// deps (the
// only kind preBuildSubchartDepsRecursive descends into). Errors are logged at
// debug level and otherwise discarded.
// acquires a per-chartDir mutex (via getNamedRWMutex) so that concurrent
// releases referencing the same local chart don't race on helm's writes to the
// chart's charts/ and Chart.lock (same pattern as forcedDownloadChart for #768).
//
// `--skip-refresh` is passed to avoid a slow `helm repo update` for every
// subchart in the tree. This is safe for file://-only deps (the common case for
// envelope charts). For subcharts that also declare https:// deps, users should
// run `helm repo update` separately; chartify's own `helm dependency build`
// (which does NOT skip refresh) runs afterwards and will catch stale repos.
//
// Errors from lock-out-of-sync are expected and logged at debug level; other
// failures (helm missing, network errors for remote deps) are logged at warn
// level so users can diagnose why patches later fail with "no resource matches".
func (st *HelmState) runHelmDependencyBuild(chartDir string) {
helmBin := st.DefaultHelmBinary
if helmBin == "" {
helmBin = "helm"
}
// Serialize per-chart-dir so concurrent releases referencing the same local
// chart tree don't corrupt charts/*.tgz or Chart.lock. The mutex is
// process-wide (rwMutexMap), matching forcedDownloadChart's approach (#768).
mu := st.getNamedRWMutex("prebuild-deps:" + chartDir)
mu.Lock()
defer mu.Unlock()
// Prefer `dependency build` (honors Chart.lock) over `dependency update`
// (re-resolves version constraints against repos, which can silently pull
// newer versions). Same strategy chartify uses internally.
@@ -1952,7 +1969,11 @@ func (st *HelmState) runHelmDependencyBuild(chartDir string) {
}
}
if err != nil {
st.logger.Debugf("preBuildSubchartDeps: helm dependency command failed for %s: %v\n%s", chartDir, err, string(output))
// Non-lock errors (helm missing, network failures, malformed Chart.yaml)
// surface as warnings so users can diagnose why strategicMergePatches
// later fail with "no resource matches". Lock-out-of-sync errors that
// also fail on `dependency update` are also warned here.
st.logger.Warnf("preBuildSubchartDeps: helm dependency command failed for %s: %v\n%s", chartDir, err, string(output))
return
}
st.logger.Debugf("preBuildSubchartDeps: built dependencies for %s", chartDir)