diff --git a/README.md b/README.md index aed9dd1e..86f99a51 100644 --- a/README.md +++ b/README.md @@ -283,8 +283,6 @@ remote: release: # ... the same command, /release: module: rules: - release-layout: - impact: error changelog-valid: impact: error ``` diff --git a/internal/manager/manager_test.go b/internal/manager/manager_test.go index 8f7f3776..383842b0 100644 --- a/internal/manager/manager_test.go +++ b/internal/manager/manager_test.go @@ -157,7 +157,7 @@ func TestRunReportsTheSourceError(t *testing.T) { // Both paths go through this function now, so one test covers both. // // The bundle scope over an empty directory is the fixture: none of the files that scope -// looks for are there, so both of its linters report — module via bundle-layout, +// looks for are there, so both of its linters report — module via has-changelog, // documentation via readme. The unfiltered case is what makes the filtered one mean // something; without it the filter would look correct even if nothing reported at all. func TestLintModuleHonoursTheLinterFilter(t *testing.T) { diff --git a/internal/modules/module.go b/internal/modules/module.go index 522bf755..b0c9a351 100644 --- a/internal/modules/module.go +++ b/internal/modules/module.go @@ -384,8 +384,6 @@ func mapModuleRules(linterSettings *pkg.LintersSettings, configSettings *config. rules.ModulePackageConsistencyRule.SetLevel(globalRules.ModulePackageConsistencyRule.Impact, fallbackImpact) rules.LegacyReleaseFileRule.SetLevel(globalRules.LegacyReleaseFileRule.Impact, fallbackImpact) rules.EnabledScriptRule.SetLevel(globalRules.EnabledScriptRule.Impact, fallbackImpact) - rules.ReleaseLayoutRule.SetLevel(globalRules.ReleaseLayoutRule.Impact, fallbackImpact) - rules.BundleLayoutRule.SetLevel(globalRules.BundleLayoutRule.Impact, fallbackImpact) rules.HelmignoreCoverageRule.SetLevel(globalRules.HelmignoreCoverageRule.Impact, fallbackImpact) rules.HasChangelogRule.SetLevel(globalRules.HasChangelogRule.Impact, fallbackImpact) rules.ChangelogValidRule.SetLevel(globalRules.ChangelogValidRule.Impact, fallbackImpact) @@ -651,8 +649,8 @@ func NewModule(path string, vals *chartutil.Values, globalSchema *spec.Schema, r // // The invariant that buys is one those scope tables have to keep — a rule reaching // for GetChart, GetObjectStore or GetValues here finds nil. name comes from the -// image reference rather than module.yaml, because a module.yaml missing from the -// image is one of the things the layout rules are there to report. +// image reference rather than module.yaml, because the image may ship no module.yaml +// at all and the module still has to be named in the findings. func NewRemoteModule(path, name string, linters *global.Linters) *Module { // The image ships no .dmtlint.yaml of its own, and a remote scope is configured // independently of the source tree: severities come from its own `remote.` diff --git a/internal/sources/static/validate.go b/internal/sources/static/validate.go index bf38dbb8..215d82e0 100644 --- a/internal/sources/static/validate.go +++ b/internal/sources/static/validate.go @@ -30,7 +30,7 @@ import ( // validateModule is the source tree's pre-flight check. It reports through the // `module`/`definition-file` rule rather than under its own name, because what it // checks is what that rule checks — the remote scopes reach the same ground through -// the bundle-layout and release-layout rules instead. +// the definition-file rule itself, which runs over the unpacked image. func validateModule(path string, errorList *dmtErrors.LintRuleErrorsList) error { var errs error diff --git a/pkg/config.go b/pkg/config.go index 9e1afd34..b67fa816 100644 --- a/pkg/config.go +++ b/pkg/config.go @@ -246,8 +246,6 @@ type ModuleLinterRules struct { ModulePackageConsistencyRule RuleConfig LegacyReleaseFileRule RuleConfig EnabledScriptRule RuleConfig - ReleaseLayoutRule RuleConfig - BundleLayoutRule RuleConfig HelmignoreCoverageRule RuleConfig HasChangelogRule RuleConfig ChangelogValidRule RuleConfig diff --git a/pkg/config/global/global.go b/pkg/config/global/global.go index 539bd70f..9daed780 100644 --- a/pkg/config/global/global.go +++ b/pkg/config/global/global.go @@ -129,8 +129,6 @@ type ModuleLinterRules struct { ModulePackageConsistencyRule RuleConfig `mapstructure:"module-package-consistency"` LegacyReleaseFileRule RuleConfig `mapstructure:"legacy-release-file"` EnabledScriptRule RuleConfig `mapstructure:"enabled-script"` - ReleaseLayoutRule RuleConfig `mapstructure:"release-layout"` - BundleLayoutRule RuleConfig `mapstructure:"bundle-layout"` HelmignoreCoverageRule RuleConfig `mapstructure:"helmignore-coverage"` HasChangelogRule RuleConfig `mapstructure:"has-changelog"` ChangelogValidRule RuleConfig `mapstructure:"changelog-valid"` diff --git a/pkg/linters/README.md b/pkg/linters/README.md index 7492c2ec..fe9b4180 100644 --- a/pkg/linters/README.md +++ b/pkg/linters/README.md @@ -101,8 +101,8 @@ list** — deliberately. The tempting check is "a scope must ask every linter for all of its rules". It held while `static` was alone and stopped holding the moment `release` and -`bundle` landed: `release-layout` belongs to a built image and never runs over a -source tree, `markdownlint` is the other way round. A check like that would push +`bundle` landed: `helmignore-coverage` belongs to a built image and never runs over +a source tree, `markdownlint` is the other way round. A check like that would push back against the very thing scopes exist to express, so the tables are written out by hand and trusted. diff --git a/pkg/linters/module/README.md b/pkg/linters/module/README.md index 7dba2f87..24ade4d3 100644 --- a/pkg/linters/module/README.md +++ b/pkg/linters/module/README.md @@ -44,7 +44,7 @@ Validates that a published image carries a `changelog.yaml` and that it is not e **Scope:** `release` and `bundle` — the published images are where a changelog has to be, and the source tree is not checked for one. Whether it parses is checked by [changelog-valid](#changelog-valid), which runs in the same two scopes. -The layout rules already report the file missing in both scopes, so this rule's own contribution is the empty-file case. +This is the only rule in either scope that reports a missing file at all, which is why it covers both the absent and the empty case. A module built into the Deckhouse monorepo needs no exemption here: it publishes no images of its own — it rides the platform release, and its changes are described by the repo-level `CHANGELOG/` — so neither scope ever runs over it. @@ -58,7 +58,7 @@ Validates the `changelog.yaml` a published image ships. **Scope:** `release` and `bundle` — both published images carry `changelog.yaml`. -A missing `changelog.yaml` is not this rule's finding: the layout rules own presence and [has-changelog](#has-changelog) owns emptiness, and this rule stays quiet when the file is absent — the same split [definition-file](#definition-file) and [package-yaml](#package-yaml) follow. +A missing `changelog.yaml` is not this rule's finding: [has-changelog](#has-changelog) owns both presence and emptiness, and this rule stays quiet when the file is absent — the same split [definition-file](#definition-file) and [package-yaml](#package-yaml) follow. --- diff --git a/pkg/linters/module/module.go b/pkg/linters/module/module.go index b303325b..8a034f0f 100644 --- a/pkg/linters/module/module.go +++ b/pkg/linters/module/module.go @@ -79,8 +79,6 @@ func (l *Module) rules() []pkg.Rule { rules.NewModulePackageConsistencyRule(m, level(cfg.Rules.ModulePackageConsistencyRule)), rules.NewLegacyReleaseFileRule(m, level(cfg.Rules.LegacyReleaseFileRule)), rules.NewEnabledScriptRule(m, level(cfg.Rules.EnabledScriptRule)), - rules.NewReleaseLayoutRule(m, level(cfg.Rules.ReleaseLayoutRule)), - rules.NewBundleLayoutRule(m, level(cfg.Rules.BundleLayoutRule)), rules.NewHelmignoreCoverageRule(m, level(cfg.Rules.HelmignoreCoverageRule)), rules.NewHasChangelogRule(m, level(cfg.Rules.HasChangelogRule)), rules.NewChangelogValidRule(m, level(cfg.Rules.ChangelogValidRule)), diff --git a/pkg/linters/module/rules/changelog_valid_test.go b/pkg/linters/module/rules/changelog_valid_test.go index 28ff7118..7a239ffa 100644 --- a/pkg/linters/module/rules/changelog_valid_test.go +++ b/pkg/linters/module/rules/changelog_valid_test.go @@ -53,7 +53,7 @@ func TestChangelogYAMLRule(t *testing.T) { content: "", }, { - // release-layout owns presence; this rule must stay quiet, which is what + // has-changelog owns presence; this rule must stay quiet, which is what // lets a scope whose image carries no changelog ask for it. name: "no changelog at all", write: false, diff --git a/pkg/linters/module/rules/helmignore_coverage.go b/pkg/linters/module/rules/helmignore_coverage.go index ecebc704..fcd77b73 100644 --- a/pkg/linters/module/rules/helmignore_coverage.go +++ b/pkg/linters/module/rules/helmignore_coverage.go @@ -70,8 +70,9 @@ func (r *CoverageRule) Check(_ context.Context) { raw, err := os.ReadFile(filepath.Join(root, helmignoreFile)) if err != nil { - // A missing .helmignore leaves nothing to compare the tree against. Its absence - // is bundle-layout's finding to report, not a second copy of it here. + // A missing .helmignore leaves nothing to compare the tree against, and reporting + // the absence is not this rule's job — the helmignore rule owns it over the source + // tree, and the bundle scope does not ask for that check at all. if os.IsNotExist(err) { return } diff --git a/pkg/linters/module/rules/helmignore_coverage_test.go b/pkg/linters/module/rules/helmignore_coverage_test.go index 85b96c63..8dc8368f 100644 --- a/pkg/linters/module/rules/helmignore_coverage_test.go +++ b/pkg/linters/module/rules/helmignore_coverage_test.go @@ -37,7 +37,7 @@ func TestCoverageRule(t *testing.T) { wantFilePaths []string }{ { - name: "no .helmignore is bundle-layout's finding, not ours", + name: "a missing .helmignore is not this rule's finding", helmignore: "", dirs: []string{"images"}, }, @@ -138,7 +138,7 @@ func TestCoverageRuleReportsAtWarn(t *testing.T) { } // TestCoverageRuleSkipsHelmignoreItself pins the entry a broad pattern would otherwise -// leave uncovered against itself: bundle-layout requires .helmignore in the package root, +// leave uncovered against itself: a bundle carries .helmignore in the package root, // and no .helmignore lists itself. func TestCoverageRuleSkipsHelmignoreItself(t *testing.T) { root := t.TempDir() diff --git a/pkg/linters/module/rules/layout.go b/pkg/linters/module/rules/layout.go deleted file mode 100644 index dd3c38bf..00000000 --- a/pkg/linters/module/rules/layout.go +++ /dev/null @@ -1,124 +0,0 @@ -/* -Copyright 2025 Flant JSC - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package rules - -import ( - "context" - "os" - "path/filepath" - - "github.com/deckhouse/dmt/pkg" - "github.com/deckhouse/dmt/pkg/errors" -) - -const ( - ReleaseLayoutRuleName = "release-layout" - BundleLayoutRuleName = "bundle-layout" -) - -// LayoutRule reports the files and directories a built image is missing from its -// package root. -// -// It exists as its own rule rather than as a flag on the rules that parse those -// files because presence is a property of the scope, not of the file: a source -// tree may legitimately lack version.json, an image may not. The parsing rules -// therefore keep returning quietly when a file is absent, and this rule is what -// the release and bundle scopes ask for to make the absence a finding. -type LayoutRule struct { - pkg.RuleMeta - - module pkg.Module - errorList *errors.LintRuleErrorsList - files []string - dirs []string -} - -var _ pkg.Rule = (*LayoutRule)(nil) - -// NewReleaseLayoutRule checks the root of a release image, which carries only the -// metadata Deckhouse reads to decide whether to install the version. -func NewReleaseLayoutRule(m pkg.Module, errorList *errors.LintRuleErrorsList) *LayoutRule { - return newLayoutRule(ReleaseLayoutRuleName, m, errorList, - []string{"module.yaml", "version.json", "changelog.yaml"}, - nil, - ) -} - -// NewBundleLayoutRule checks the root of a bundle image, which carries the whole -// packaged module — chart, templates and docs included. -// -// The list is what a published bundle actually holds, which is not what its source -// tree holds: version.json ships in the sibling release image, and the ignore file -// the package carries is .helmignore, not .gitignore. -// -// It is the intersection of eight published CE bundles, not every path they carry — -// crds/, hooks/, monitoring/ and .werf/ appear in some and not others, so requiring -// any of them would fail the modules that legitimately have nothing to put there. -func NewBundleLayoutRule(m pkg.Module, errorList *errors.LintRuleErrorsList) *LayoutRule { - return newLayoutRule(BundleLayoutRuleName, m, errorList, - []string{".helmignore", "Chart.yaml", ChangelogFilename, "images_digests.json", "module.yaml"}, - []string{"charts", "docs", "openapi", "templates"}, - ) -} - -func newLayoutRule(name string, m pkg.Module, errorList *errors.LintRuleErrorsList, files, dirs []string) *LayoutRule { - return &LayoutRule{ - RuleMeta: pkg.RuleMeta{Name: name}, - module: m, - errorList: errorList.WithRule(name), - files: files, - dirs: dirs, - } -} - -func (r *LayoutRule) Check(_ context.Context) { - root := r.module.GetPath() - if root == "" { - return - } - - for _, name := range r.files { - r.check(root, name, false) - } - - for _, name := range r.dirs { - r.check(root, name, true) - } -} - -// check reports the three outcomes apart: the path is absent, it exists but is of -// the wrong kind, or it could not be read at all. -func (r *LayoutRule) check(root, name string, wantDir bool) { - kind := "file" - if wantDir { - kind = "directory" - } - - path := filepath.Join(root, name) - errorList := r.errorList.WithFilePath(name) - - info, err := os.Stat(path) - - switch { - case os.IsNotExist(err): - errorList.Errorf("%s %s is missing in package root", name, kind) - case err != nil: - errorList.WithValue(err.Error()).Errorf("failed to check %s %s", name, kind) - case info.IsDir() != wantDir: - errorList.Errorf("%s must be a %s in package root", name, kind) - } -} diff --git a/pkg/linters/module/rules/layout_test.go b/pkg/linters/module/rules/layout_test.go deleted file mode 100644 index e641f85c..00000000 --- a/pkg/linters/module/rules/layout_test.go +++ /dev/null @@ -1,99 +0,0 @@ -/* -Copyright 2025 Flant JSC - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package rules - -import ( - "context" - "os" - "path/filepath" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/deckhouse/dmt/pkg" - "github.com/deckhouse/dmt/pkg/errors" -) - -func TestLayoutRules(t *testing.T) { - releaseFiles := []string{"module.yaml", "version.json", "changelog.yaml"} - // The bundle fixture is the root of a real published bundle, not a copy of the - // rule's own list — deriving it from the rule would only prove the rule agrees - // with itself. Eight CE bundles (sds-node-configurator, sds-local-volume, - // sds-replicated-volume, csi-nfs, console, commander-agent, observability, - // secrets-store-integration) all carry these, and differ only in the optional - // crds/, hooks/, monitoring/ and .werf/. - bundleFiles := []string{".helmignore", "Chart.yaml", "changelog.yaml", "images_digests.json", "module.yaml"} - bundleDirs := []string{"charts", "docs", "openapi", "templates"} - - t.Run("release layout is complete", func(t *testing.T) { - root := layoutAt(t, releaseFiles, nil) - - assert.Empty(t, checkLayout(t, NewReleaseLayoutRule, root)) - }) - - t.Run("bundle layout is complete", func(t *testing.T) { - root := layoutAt(t, bundleFiles, bundleDirs) - - assert.Empty(t, checkLayout(t, NewBundleLayoutRule, root)) - }) - - t.Run("a missing file is one finding", func(t *testing.T) { - root := layoutAt(t, releaseFiles, nil) - require.NoError(t, os.Remove(filepath.Join(root, "version.json"))) - - errs := checkLayout(t, NewReleaseLayoutRule, root) - - require.Len(t, errs, 1) - assert.Contains(t, errs[0].Text, "version.json file is missing") - }) - - t.Run("the wrong kind is reported as such", func(t *testing.T) { - // docs is a directory in a bundle; a file by that name is not the same thing. - root := layoutAt(t, append(bundleFiles, "docs"), []string{"charts", "openapi", "templates"}) - - errs := checkLayout(t, NewBundleLayoutRule, root) - - require.Len(t, errs, 1) - assert.Contains(t, errs[0].Text, "docs must be a directory") - }) -} - -func checkLayout(t *testing.T, newRule func(pkg.Module, *errors.LintRuleErrorsList) *LayoutRule, root string) []pkg.LinterError { - t.Helper() - - errorList := errors.NewLintRuleErrorsList() - newRule(moduleAt(t, root), errorList).Check(context.Background()) - - return errorList.GetErrors() -} - -func layoutAt(t *testing.T, files, dirs []string) string { - t.Helper() - - root := t.TempDir() - - for _, name := range files { - require.NoError(t, os.WriteFile(filepath.Join(root, name), []byte("x"), DefaultFilePerm)) - } - - for _, name := range dirs { - require.NoError(t, os.Mkdir(filepath.Join(root, name), DefaultDirPerm)) - } - - return root -} diff --git a/pkg/scopes/bundle.go b/pkg/scopes/bundle.go index af214986..672bb912 100644 --- a/pkg/scopes/bundle.go +++ b/pkg/scopes/bundle.go @@ -35,7 +35,6 @@ import ( // would work off a chart that was never loaded. var bundleRules = map[string]set.Set{ moduleLinter.ID: set.New( - modulerules.BundleLayoutRuleName, modulerules.HelmignoreCoverageRuleName, modulerules.HasChangelogRuleName, modulerules.ChangelogValidRuleName, diff --git a/pkg/scopes/release.go b/pkg/scopes/release.go index ef72c049..48772d90 100644 --- a/pkg/scopes/release.go +++ b/pkg/scopes/release.go @@ -31,14 +31,13 @@ import ( // and for nothing that needs a chart or a rendered object — the module behind this // scope is built by modules.NewRemoteModule and has neither. // -// release-layout is what makes a missing file a finding. definition-file, -// package-yaml and changelog-valid validate the contents of module.yaml, package.yaml -// and changelog.yaml and stay quiet when the file is absent, which is why the presence -// check is a rule of its own. package.yaml is not in the layout list on purpose: it is -// validated when the image ships one, and its absence is not an error. +// No rule here turns a missing file into an error. definition-file, package-yaml and +// changelog-valid validate the contents of module.yaml, package.yaml and changelog.yaml +// and stay quiet when the file is absent; has-changelog is the only one that reports an +// absence at all, and it warns. So this scope checks what the image ships, not what it +// forgot to ship — an empty release image comes back with a single changelog warning. var releaseRules = map[string]set.Set{ moduleLinter.ID: set.New( - modulerules.ReleaseLayoutRuleName, modulerules.DefinitionFileRuleName, modulerules.PackageYAMLRuleName, modulerules.HasChangelogRuleName, diff --git a/pkg/scopes/scopes_test.go b/pkg/scopes/scopes_test.go index be892fc2..9e8e6796 100644 --- a/pkg/scopes/scopes_test.go +++ b/pkg/scopes/scopes_test.go @@ -86,14 +86,20 @@ func TestRemoteScopesRunOverAnUnpackedImage(t *testing.T) { wantErr bool }{ {scope: Release, files: []string{"module.yaml", "version.json", "changelog.yaml"}}, - {scope: Release, wantErr: true}, + // An empty release image is not an error: every rule of that scope validates the + // contents of a file it finds and stays quiet when the file is absent. The case is + // still here for the panic it would catch, not for the findings it does not make. + {scope: Release}, { scope: Bundle, - // The root of a real published bundle — see layout_test.go for where it - // comes from. Deriving it from bundleRules would test nothing. + // The root of a real published bundle: the intersection of eight published CE + // bundles, which is not what a source tree holds — version.json ships in the + // sibling release image, and the ignore file a package carries is .helmignore. + // Deriving this from bundleRules would test nothing. files: []string{".helmignore", "Chart.yaml", "changelog.yaml", "images_digests.json", "module.yaml"}, dirs: []string{"charts", "docs", "openapi", "templates"}, }, + // The bundle scope still errors on an empty root, through the readme rule. {scope: Bundle, wantErr: true}, } { name := string(tc.scope) + " complete" @@ -111,9 +117,8 @@ func TestRemoteScopesRunOverAnUnpackedImage(t *testing.T) { require.NoError(t, os.Mkdir(filepath.Join(root, d), 0o755)) } - // The layout rules only want the paths to exist, but definition-file parses - // module.yaml and the readme rule reads docs/README.md, so the complete - // cases need real content in both. + // definition-file parses module.yaml and the readme rule reads docs/README.md, + // so the complete cases need real content in both. if len(tc.files) > 0 { require.NoError(t, os.WriteFile(filepath.Join(root, "module.yaml"), []byte("name: test-module\nstage: General Availability\ndescriptions:\n en: a module\n"), 0o600)) diff --git a/test/e2e/testdata/module/helmignore-coverage-uncovered/expected.yaml b/test/e2e/testdata/module/helmignore-coverage-uncovered/expected.yaml index 3f941cc0..08736661 100644 --- a/test/e2e/testdata/module/helmignore-coverage-uncovered/expected.yaml +++ b/test/e2e/testdata/module/helmignore-coverage-uncovered/expected.yaml @@ -5,8 +5,8 @@ description: > Chart material (templates/, charts/, Chart.yaml) and the build-generated images_digests.json need no pattern and must stay unreported, as must hooks/, openapi/, docs/ and module.yaml, which the patterns do cover. docs/ is the - interesting one: bundle-layout requires it in the root, it is not chart - material, so it needs a pattern like any other shipped non-chart entry. + interesting one: every bundle ships it in the root, it is not chart material, + so it needs a pattern like any other shipped non-chart entry. kind: bundle module: module expect: