fix: address Copilot review comments round 3

- Log parse errors from SelectorsAreCompatible at debug level instead of
  silently discarding them
- Hoist regex compilation to package-level vars in ParseLabels to avoid
  repeated compilation per selector
- Replace EXIT traps with explicit cleanup calls in integration test to
  avoid interfering with the parent runner's trap

Signed-off-by: yxxhero <aiopsclub@163.com>

pkg/app/app.go

@@ -1007,7 +1007,7 @@ func (a *App) processStateFileParallel(relPath string, defOpts LoadOpts, converg
 func (a *App) processNestedHelmfiles(st *state.HelmState, absd, file string, defOpts, opts LoadOpts, converge func(*state.HelmState) (bool, []error), sharedCtx *Context) (bool, error) {
 	anyMatched := false
 	for i, m := range st.Helmfiles {
-		if subhelmfileSelectorsConflict(a.Selectors, m) {
+		if subhelmfileSelectorsConflict(a.Selectors, m, a.Logger) {
 			a.Logger.Debugf("skipping subhelmfile %q: CLI selectors %v conflict with subhelmfile selectors %v", m.Path, a.Selectors, m.Selectors)
 			continue
 		}
@@ -1044,11 +1044,14 @@ func (a *App) processNestedHelmfiles(st *state.HelmState, absd, file string, def
 // Only CLI selectors (not inherited parent selectors) are used for comparison,
 // so this optimization is restricted to cases where the user explicitly
 // provided selectors via the command line (e.g. -l name=b).
-func subhelmfileSelectorsConflict(cliSelectors []string, m state.SubHelmfileSpec) bool {
+func subhelmfileSelectorsConflict(cliSelectors []string, m state.SubHelmfileSpec, logger *zap.SugaredLogger) bool {
 	if len(cliSelectors) == 0 || len(m.Selectors) == 0 || m.SelectorsInherited {
 		return false
 	}
-	compatible, _ := state.SelectorsAreCompatible(cliSelectors, m.Selectors)
+	compatible, err := state.SelectorsAreCompatible(cliSelectors, m.Selectors)
+	if err != nil {
+		logger.Debugf("selector compatibility check failed for subhelmfile %q: %v", m.Path, err)
+	}
 	return !compatible
 }
 

pkg/app/app_test.go

@@ -928,7 +928,7 @@ func TestSubhelmfileSelectorsConflict(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := subhelmfileSelectorsConflict(tt.cliSelectors, tt.spec)
+			got := subhelmfileSelectorsConflict(tt.cliSelectors, tt.spec, newAppTestLogger())
 			assert.Equal(t, tt.conflict, got)
 		})
 	}

pkg/state/release_filters.go

@@ -105,6 +105,11 @@ func (l LabelFilter) positiveLabelsCompatibleWith(other LabelFilter) bool {
 	return true
 }
 
+var (
+	reLabelMismatch = regexp.MustCompile(`^[a-zA-Z0-9_\.\/\+-]+!=[a-zA-Z0-9_\.\/\+-]+$`)
+	reLabelMatch    = regexp.MustCompile(`^[a-zA-Z0-9_\.\/\+-]+=[a-zA-Z0-9_\.\/\+-]+$`)
+)
+
 // ParseLabels takes a label in the form foo=bar,baz!=bat and returns a LabelFilter that will match the labels
 func ParseLabels(l string) (LabelFilter, error) {
 	lf := LabelFilter{}
@@ -112,16 +117,14 @@ func ParseLabels(l string) (LabelFilter, error) {
 	lf.negativeLabels = [][]string{}
 	var err error
 	labels := strings.Split(l, ",")
-	reMissmatch := regexp.MustCompile(`^[a-zA-Z0-9_\.\/\+-]+!=[a-zA-Z0-9_\.\/\+-]+$`)
-	reMatch := regexp.MustCompile(`^[a-zA-Z0-9_\.\/\+-]+=[a-zA-Z0-9_\.\/\+-]+$`)
 	for _, label := range labels {
-		if match := reMissmatch.MatchString(label); match { // k!=v case
+		if match := reLabelMismatch.MatchString(label); match {
 			kv := strings.Split(label, "!=")
 			lf.negativeLabels = append(lf.negativeLabels, kv)
-		} else if match := reMatch.MatchString(label); match { // k=v case
+		} else if match := reLabelMatch.MatchString(label); match {
 			kv := strings.Split(label, "=")
 			lf.positiveLabels = append(lf.positiveLabels, kv)
-		} else { // malformed case
+		} else {
 			return lf, fmt.Errorf("malformed label: %s. Expected label in form k=v or k!=v", label)
 		}
 	}

test/integration/test-cases/issue-2544.sh

@@ -9,7 +9,6 @@ issue_2544_tmp_dir=$(mktemp -d)
 cleanup_issue_2544() {
     rm -rf "${issue_2544_tmp_dir}"
 }
-trap cleanup_issue_2544 EXIT
 
 test_start "issue-2544: subhelmfiles skipped when selectors conflict with CLI"
 
@@ -19,19 +18,20 @@ info "Running helmfile template -l app=b"
 ${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template -l app=b \
     > "${issue_2544_tmp_dir}/output-b.txt" 2>&1 || {
     cat "${issue_2544_tmp_dir}/output-b.txt"
+    cleanup_issue_2544
     fail "helmfile template -l app=b failed"
 }
 
 # Should contain release-b
 grep -q "release-b" "${issue_2544_tmp_dir}/output-b.txt" \
-    || fail "output should contain release-b (matching selector app=b)"
+    || { cleanup_issue_2544; fail "output should contain release-b (matching selector app=b)"; }
 
 # Should NOT contain release-a or release-c (their subhelmfiles have incompatible selectors)
 grep -q "release-a" "${issue_2544_tmp_dir}/output-b.txt" \
-    && fail "output should NOT contain release-a (subhelmfile selectors app=a conflict with app=b)"
+    && { cleanup_issue_2544; fail "output should NOT contain release-a (subhelmfile selectors app=a conflict with app=b)"; }
 
 grep -q "release-c" "${issue_2544_tmp_dir}/output-b.txt" \
-    && fail "output should NOT contain release-c (subhelmfile selectors app=c conflict with app=b)"
+    && { cleanup_issue_2544; fail "output should NOT contain release-c (subhelmfile selectors app=c conflict with app=b)"; }
 
 info "PASS: -l app=b produces only release-b"
 
@@ -41,15 +41,16 @@ info "Running helmfile template without selector"
 ${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template \
     > "${issue_2544_tmp_dir}/output-all.txt" 2>&1 || {
     cat "${issue_2544_tmp_dir}/output-all.txt"
+    cleanup_issue_2544
     fail "helmfile template without selector failed"
 }
 
 grep -q "release-a" "${issue_2544_tmp_dir}/output-all.txt" \
-    || fail "output should contain release-a"
+    || { cleanup_issue_2544; fail "output should contain release-a"; }
 grep -q "release-b" "${issue_2544_tmp_dir}/output-all.txt" \
-    || fail "output should contain release-b"
+    || { cleanup_issue_2544; fail "output should contain release-b"; }
 grep -q "release-c" "${issue_2544_tmp_dir}/output-all.txt" \
-    || fail "output should contain release-c"
+    || { cleanup_issue_2544; fail "output should contain release-c"; }
 
 info "PASS: no selector produces all releases"
 
@@ -59,19 +60,20 @@ info "Running helmfile template -l app=a"
 ${helmfile} -f "${issue_2544_input_dir}/helmfile.yaml" template -l app=a \
     > "${issue_2544_tmp_dir}/output-a.txt" 2>&1 || {
     cat "${issue_2544_tmp_dir}/output-a.txt"
+    cleanup_issue_2544
     fail "helmfile template -l app=a failed"
 }
 
 grep -q "release-a" "${issue_2544_tmp_dir}/output-a.txt" \
-    || fail "output should contain release-a (matching selector app=a)"
+    || { cleanup_issue_2544; fail "output should contain release-a (matching selector app=a)"; }
 
 grep -q "release-b" "${issue_2544_tmp_dir}/output-a.txt" \
-    && fail "output should NOT contain release-b (subhelmfile selectors app=b conflict with app=a)"
+    && { cleanup_issue_2544; fail "output should NOT contain release-b (subhelmfile selectors app=b conflict with app=a)"; }
 
 grep -q "release-c" "${issue_2544_tmp_dir}/output-a.txt" \
-    && fail "output should NOT contain release-c (subhelmfile selectors app=c conflict with app=a)"
+    && { cleanup_issue_2544; fail "output should NOT contain release-c (subhelmfile selectors app=c conflict with app=a)"; }
 
 info "PASS: -l app=a produces only release-a"
 
-trap - EXIT
+cleanup_issue_2544
 test_pass "issue-2544: subhelmfiles skipped when selectors conflict with CLI"