Repository navigation
feat(components): dedupe nested chart Components in component linter - #1034
Merged
VLukiantsev merged 5 commits intoOct 2, 2026
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request renames the xmc-linter binary to component-linter across documentation, tests, and scripts. It also introduces logic to skip nested Helm chart roots during directory traversal in the linter to prevent duplicate manifest rendering, while still allowing vendored subcharts to be linted individually. Feedback on the changes suggests refactoring the isNestedChartRoot function to resolve absolute paths upfront, which simplifies error handling and avoids redundant path resolution.
VLukiantsev
marked this pull request as ready for review
September 29, 2026 11:36
pachopepe
reviewed
Sep 29, 2026
VLukiantsev
force-pushed
the
feat/components-linter-nested-chart-roots-b548435909
branch
from
October 2, 2026 08:16
0174cbd to
fc94dc3
Compare
A profile meta-chart renders its co-located component charts through .Files, so walking into components/<name>/ and rendering those charts again would lint every Component twice and report DUPLICATE_COMPONENT_NAME. collectManifests now skips a directory whose Chart.yaml sits inside another chart root at or below --manifests-dir. The check is relative to the walk root, so pointing --manifests-dir at a single component chart, or at a chart-free directory of component charts, still renders them individually. Vendored subcharts directly under <chart>/charts/ are not treated as nested and are still linted on their own. findChartRoot's Chart.yaml probe is extracted into hasChartFile so the new isNestedChartRoot helper can reuse it. BUG=b/548435909 TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
The linter binary and Bazel package are component-linter (//cmd/component-linter), but the component-linter README still referred to an xmc-linter binary and //cmd/xmc-linter targets that do not exist, so the documented bazel run/test/coverage commands failed. The ingestion-gateway component README, the run_linter.py docstring, and the fake binary names in run_linter_test.py used the same stale name. BUG=b/548435909 TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
Resolve the absolute chart directory once, up front, with an early return on error, and pass it to findChartRoot instead of resolving it twice. The function now reads as a linear sequence of guard clauses: chart-file check, path resolution, enclosing-chart lookup, vendored charts/ exemption, and the walk-root containment check. findChartRoot starts at the parent directory, so it still returns the enclosing chart; behavior is unchanged. Add a table-driven TestIsNestedChartRoot covering each reachable branch. Comment link: #1034 (comment) BUG=b/548435909 TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
Stop skipping chart roots nested under an enclosing chart root. Every nested template is rendered again with the nested chart's own defaults, so template errors surface as YAML_SYNTAX. A nested Component is dropped only when the enclosing chart already renders a Component with the same canonical name, which avoids DUPLICATE_COMPONENT_NAME. Nested Components the enclosing chart does not render, and all other nested documents, are linted as usual. The nested-chart test fixture now renders its nested chart through .Files and tpl. It asserts the exact set of diagnostics, and covers a nested chart the outer chart does not include and a nested template that fails to compile. Comment link: #1034 (comment) Comment link: #1034 (comment) TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
Describe the new nested-chart rule in the component-linter README. Note that diagnostics for Components rendered by an enclosing meta-chart point at the meta-chart's template, with line numbers from the rendered output. Recommend a per-document source comment so these locations are easy to trace. Drop the advice to lint nested charts on their own, which CI never followed. Comment link: #1034 (comment) TAG=agy CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954
VLukiantsev
force-pushed
the
feat/components-linter-nested-chart-roots-b548435909
branch
from
October 2, 2026 12:11
fc94dc3 to
d16b0b9
Compare
pachopepe
approved these changes
Oct 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
component-linternow handles chart roots nested inside another chart root under--manifests-dir. Nested charts are still rendered, so their template errors surface. A nestedComponentis dropped only when the enclosing chart already renders aComponentwith the same canonical name. This lets a profile meta-chart that renders its co-located component charts through.Filesandtplbe linted without reporting everyComponenttwice asDUPLICATE_COMPONENT_NAME. The PR also fixes stalexmc-linterreferences.Motivation & Context
The next PR in this stack (#1016) turns
deploy/profiles/stable-2026a/into a Helm chart whose templates render eachcomponents/<name>/templates/component.yaml. Without this change, walking the profile renders eachComponenttwice: once from the profile chart and once from the component chart.Helm loads only the enclosing chart's own
templates/as templates, and everything undercomponents/is plain.Files. Skipping nested charts entirely would therefore hide errors in nested templates that the meta-chart never reads, such as a component'stemplates/deployment.yaml, and in Components the meta-chart doesn't include. So nested charts keep being rendered on their own, and only the duplicated Components are dropped.Which charts count as nested is decided relative to the walk root:
--manifests-dirat a single component chart, or at a directory of charts that has no chart of its own, lints each chart exactly as before.<chart>/charts/are not treated as nested.Note
Reachability: no chart root in the repository is nested inside another one until #1016 lands, so on
mainthis behavior is inert.run_linter.pyand therun-component-linteraction are unchanged. The new path is exercised by unit tests on temporary chart trees.Key Changes
pkg/components/linter):collectManifestsputs manifests from nested chart roots in a separate set. Nested charts are detected byisNestedChartRoot, which usesfilepath.IsLocaland exempts<chart>/charts/<sub>. File reading and rendering moved intoreadManifestFile, and the YAML file filter intoisManifestFile.mergeNestedManifestsappends nested manifests to the top-level ones. It drops a nestedComponent(comp and raw doc) only when a top-level chart renders the same canonical name.findChartRoot'sChart.yamlprobe is extracted intohasChartFileso both paths share it.TestValidateDirectory_NestedChartRoots: one shared tree with 8 cases, each comparing the full sorted list of<rule> <file>diagnostics. The tree contains:tpl (.Files.Get ...): oneMISSING_DEPENDENCYon the outer template, noDUPLICATE_COMPONENT_NAME;YAML_SYNTAX;charts/sub;dup.TestMergeNestedManifests, covering shadowed, kept, unnamed and non-Component documents.TestHasChartFileandTestIsNestedChartRoot. Thelinter_testBUILD target usesembedfor the internal tests.cmd/component-linter/README.mddocuments the nested-chart rule. It also notes that diagnostics for aComponentrendered by the enclosing chart point at the enclosing template, with line numbers from the rendered output, and recommends a per-document source comment in meta-charts.xmc-linter///cmd/xmc-linterreferences are fixed in that README, theingestion-gatewaycomponent README, therun_linter.pydocstring, and the fake binary names inrun_linter_test.py. The stale README references made the documented Bazel commands fail.Assumptions
--manifests-dir, not the filesystem root. Why: callers that point the linter directly at a component chart must keep linting it unchanged. Impact if wrong: none functionally, because nested charts are still rendered; only the deduplication would differ.<chart>/charts/count as vendored subcharts. Why: that is Helm's layout for dependencies. Impact if wrong: Components in deeper vendored layouts that share a name with a top-level Component would be deduplicated. No such layout exists in the repository.metadata.nameonly. Why: the enclosing chart's copy carries the values that are actually installed. Impact if wrong: a nested Component that the meta-chart renders under a different name is linted twice, with nested defaults and with profile values.filepath.Abserror branches incollectManifestsandisNestedChartRootare not covered by tests. They only fail if the working directory cannot be resolved.Verification & Testing
bazel test //pkg/components/linter/... //tools/components/... //cmd/component-linter/...→ 3/3 pass.//test/helm/profiles/stable2026a/...pass.component-linterondeploy/profiles/stable-2026awith itsrules.yamlpasses. With a stray{{ end }}added to a copy ofcomponents/ingestion-gateway/templates/deployment.yaml, it reportsYAML_SYNTAX.python3 tools/components/run_linter.py --strict(default scan roots) and--scan-roots deploy/profiles --strictgave byte-identical output for binaries built from this branch and frommain.filepath.Absand walk error branches. Pre-commit (buildifier, gofmt, golangci-lint, addlicense, ruff) is clean.Related Issues / Bugs
Part of bug 548435909
TAG=agy
CONV=bd17ce9f-7baf-43c9-8f30-c169fd656954