diff --git a/cmd/mesh-controller/check.go b/cmd/mesh-controller/check.go index 4ccf7b5f..16e16ba6 100644 --- a/cmd/mesh-controller/check.go +++ b/cmd/mesh-controller/check.go @@ -75,6 +75,16 @@ func moduleCheckFor(paths []string, longestMachine int, out io.Writer) error { failed += len(wrong) faulted[m.Module] = true } + // A module that declares a setting names the keys of its mergeable files (novox/hq ADR 0326 §3, + // issue 574): judged where a definition is written, as at registration, never where a stored + // build is read again. + if wrong := catalogue.WrittenSettingsProblems(m); len(wrong) > 0 { + for _, p := range wrong { + fmt.Fprintf(out, "%s: %s\n", path, p) + } + failed += len(wrong) + faulted[m.Module] = true + } if named := catalogue.InstallationProblems(m); len(named) > 0 { for _, p := range named { fmt.Fprintf(out, "%s: %s\n", path, p) diff --git a/cmd/mesh-controller/modules.go b/cmd/mesh-controller/modules.go index 9929b2e0..f2d60f4b 100644 --- a/cmd/mesh-controller/modules.go +++ b/cmd/mesh-controller/modules.go @@ -1051,6 +1051,11 @@ func namesNoInstallation(m catalogue.Manifest) error { if problems := catalogue.TrustProblems(m); len(problems) > 0 { return fmt.Errorf("%s", strings.Join(problems, "; ")) } + // And a module that declares a setting names the keys of its mergeable files (novox/hq ADR 0326 + // §3, issue 574): judged where a definition is written, never where a stored build is read again. + if problems := catalogue.WrittenSettingsProblems(m); len(problems) > 0 { + return fmt.Errorf("%s", strings.Join(problems, "; ")) + } named := catalogue.InstallationProblems(m) if len(named) == 0 { return nil diff --git a/cmd/mesh-controller/written_settings_test.go b/cmd/mesh-controller/written_settings_test.go new file mode 100644 index 00000000..a0a61224 --- /dev/null +++ b/cmd/mesh-controller/written_settings_test.go @@ -0,0 +1,46 @@ +package main + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/novox/mesh-controller/internal/catalogue" +) + +// ADR 0326 §3 is judged where a definition is written (novox/hq issue 574): `module check`, and +// namesNoInstallation, which `module add`, a build taken in and the merge gate's registration run. +func TestWhereADefinitionIsWrittenADeclaringModuleNamesItsFilesKeys(t *testing.T) { + const silent = `{"module":"mail-client","version":"1", + "settings":{"prefs":{"kind":"preference","default":"// none","why":"its own defaults until a machine names one"}}, + "resources":[ + {"id":"user-js","type":"file","path":"/var/lib/mail-client/user.js","mode":"0644","content":"${setting:prefs}\n"}, + {"id":"accounts","type":"file","path":"/var/lib/mail-client/accounts.json","mode":"0600","merge":"json", + "content":"{}\n"%s}]}` + dir := t.TempDir() + bad := filepath.Join(dir, "bad.json") + good := filepath.Join(dir, "good.json") + os.WriteFile(bad, []byte(strings.Replace(silent, "%s", "", 1)), 0o600) + os.WriteFile(good, []byte(strings.Replace(silent, "%s", `,"takes-settings":["accounts"]`, 1)), 0o600) + + var out bytes.Buffer + if err := moduleCheck([]string{bad}, &out); err == nil || + !strings.Contains(out.String(), "mergeable accounts names no takes-settings") { + t.Fatalf("module check passed a declaring module's silent mergeable file: %v\n%s", err, out.String()) + } + out.Reset() + if err := moduleCheck([]string{good}, &out); err != nil { + t.Fatalf("the file naming its keys was refused: %v\n%s", err, out.String()) + } + + raw, _ := os.ReadFile(bad) + m, err := catalogue.ParseManifest(raw) + if err != nil { + t.Fatalf("reading it is not where it is judged: %v", err) + } + if err := namesNoInstallation(m); err == nil || !strings.Contains(err.Error(), "names no takes-settings") { + t.Fatalf("registration passed it: %v", err) + } +} diff --git a/internal/builder/builder.go b/internal/builder/builder.go index 5ce8ae9c..b64856f3 100644 --- a/internal/builder/builder.go +++ b/internal/builder/builder.go @@ -226,6 +226,13 @@ func build(ctx context.Context, run Runner, publish Publisher, say("manifest", "INVALID: %v", err) return Result{}, err } + // What is judged only where a definition is written (novox/hq ADR 0326 §3, issue 574): a build is + // where a changed definition is first read, so it is refused here before anything is made of it. + if problems := catalogue.WrittenSettingsProblems(manifest); len(problems) > 0 { + err := fmt.Errorf("%s", strings.Join(problems, "; ")) + say("manifest", "INVALID: %v", err) + return Result{}, err + } say("manifest", "%s v%s — %d artifact(s)", manifest.Module, manifest.Version, artifactCount(manifest)) // What it is made from, for its source fingerprint: the module's own tree first. diff --git a/internal/catalogue/setting_takes.go b/internal/catalogue/setting_takes.go index 950f69d0..0cb7fca4 100644 --- a/internal/catalogue/setting_takes.go +++ b/internal/catalogue/setting_takes.go @@ -70,21 +70,38 @@ func namesItsKeys(m Manifest) bool { // reads itself, one the file keeps for itself, or one mergeable file naming its keys beside another // that names none. func TakenSettingsProblems(m Manifest) []string { - problems := takenSettingsProblems(m) - // **A module that declares a setting names the keys of every mergeable file** (ADR 0326 §3, ADR - // 0164 §11 and §15; novox/hq issue 574): only a module that declares none keeps today's form. Said - // where a definition is checked and registered, never where a stored one is composed: a definition - // stored before this rule is brought into the form by its next change, not left out of a machine - // meanwhile. - if len(m.Settings) > 0 { - for _, r := range m.Resources { - how, _ := r["merge"].(string) - if _, said := r[TakesSettingsField]; how != "" && !said { - problems = append(problems, fmt.Sprintf("%s declares settings, and its mergeable %v names no %s: "+ - "a module that declares a setting names the keys each mergeable file takes, an empty list "+ - "for none (novox/hq ADR 0326 §3, ADR 0164 §11)", m.Module, r["id"], TakesSettingsField)) - } + return takenSettingsProblems(m) +} + +// WrittenSettingsProblems is ADR 0326 §3, judged only where a definition is written — the module +// check, `module add`, a build taken in, the builder and the merge gate's registration — and never +// where a stored one is read again (ParseManifest, a build's manifest at a commit, a module put back): +// a module that declares a setting names the keys of every mergeable file it owns (novox/hq issue +// 574, ADR 0164 §11 and §15). A build stored before the rule, such as betterbird's before its accounts +// file named its keys, must still compose and still be put back; only its next change is held to it. +// +// Said only when none of the module's mergeable files names its keys: when one does, §2 already +// names every file that does not, and a file is not reported twice. +func WrittenSettingsProblems(m Manifest) []string { + if len(m.Settings) == 0 { + return nil + } + var silent []string + for _, r := range m.Resources { + how, _ := r["merge"].(string) + if how == "" { + continue } + if _, said := r[TakesSettingsField]; said { + return nil + } + silent = append(silent, fmt.Sprint(r["id"])) + } + var problems []string + for _, id := range silent { + problems = append(problems, fmt.Sprintf("%s declares settings, and its mergeable %s names no %s: "+ + "a module that declares a setting names the keys each mergeable file takes, an empty list "+ + "for none (novox/hq ADR 0326 §3, ADR 0164 §11)", m.Module, id, TakesSettingsField)) } return problems } diff --git a/internal/catalogue/setting_takes_test.go b/internal/catalogue/setting_takes_test.go index a4503cea..3deb6a6c 100644 --- a/internal/catalogue/setting_takes_test.go +++ b/internal/catalogue/setting_takes_test.go @@ -197,26 +197,47 @@ func containsText(list []string, text string) bool { func TestAModuleThatDeclaresASettingNamesItsFilesKeys(t *testing.T) { // ADR 0326 §3, ADR 0164 §15, novox/hq issue 574: only a module that declares no settings keeps - // today's form. One that declares a preference and owns a silent mergeable file is refused where it - // is checked — and not where a stored definition is composed, so a machine does not lose it before - // its next change. - m := twoFiles(nil, nil) - m.Settings = map[string]SettingDeclaration{"site-name": {Kind: KindPreference, Default: "Search", Why: "a name"}} - problems := TakenSettingsProblems(m) - if len(problems) != 2 || !containsText(problems, "app names no takes-settings") || - !containsText(problems, "sidecar names no takes-settings") { - t.Errorf("problems: %v", problems) + // today's form. One that declares a preference and owns a silent mergeable file is refused where + // a definition is written — and read and composed where a stored one is read again, so a build kept + // from before the rule is still sent and still put back. + declaring := func(app, sidecar []any) Manifest { + m := twoFiles(app, sidecar) + m.Resources[2]["content"] = "WORKERS=${setting:workers}\n" + m.Settings = map[string]SettingDeclaration{"workers": {Kind: KindPreference, Default: 4.0, Why: "a number"}} + return m } - if err := JudgeSettings(m, nil, false); err != nil { + m := declaring(nil, nil) + problems := WrittenSettingsProblems(m) + if len(problems) != 2 || !containsText(problems, "mergeable app names no takes-settings") || + !containsText(problems, "mergeable sidecar names no takes-settings") { + t.Errorf("where it is written: %v", problems) + } + if problems := TakenSettingsProblems(m); len(problems) != 0 { + t.Errorf("what a stored definition is read by refuses it: %v", problems) + } + raw, err := json.Marshal(map[string]any{"module": m.Module, "version": "1", "resources": m.Resources, + "settings": map[string]any{"workers": map[string]any{"kind": KindPreference, "default": 4, "why": "a number"}}}) + if err != nil { + t.Fatal(err) + } + read, err := ParseManifest(raw) + if err != nil { + t.Fatalf("a stored build from before the rule is not read again: %v", err) + } + if err := JudgeSettings(read, nil, false); err != nil { t.Errorf("a stored definition is composed as before, and was refused: %v", err) } - m = twoFiles([]any{"server"}, []any{}) - m.Settings = map[string]SettingDeclaration{"site-name": {Kind: KindPreference, Default: "Search", Why: "a name"}} - if problems := TakenSettingsProblems(m); len(problems) != 0 { + + if problems := WrittenSettingsProblems(declaring([]any{"server"}, []any{})); len(problems) != 0 { t.Errorf("every mergeable file names its keys, and was refused: %v", problems) } - // And a module that declares none keeps today's form, silent files and all. - if problems := TakenSettingsProblems(twoFiles(nil, nil)); len(problems) != 0 { + if problems := WrittenSettingsProblems(twoFiles(nil, nil)); len(problems) != 0 { t.Errorf("a module declaring no setting was refused: %v", problems) } + // One naming file and one silent: §2 names the silent one, and §3 does not name it again. + mixed := declaring([]any{"server"}, nil) + if written, taken := WrittenSettingsProblems(mixed), TakenSettingsProblems(mixed); len(written) != 0 || len(taken) != 1 { + t.Errorf("the silent file is reported %d times by §3 and %d by §2; want once, by §2: %v %v", + len(written), len(taken), written, taken) + } } diff --git a/internal/inventory/stored_silent_file_test.go b/internal/inventory/stored_silent_file_test.go new file mode 100644 index 00000000..d5029b9e --- /dev/null +++ b/internal/inventory/stored_silent_file_test.go @@ -0,0 +1,67 @@ +package inventory + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/novox/mesh-controller/internal/catalogue" +) + +// A build kept from before ADR 0326 §3 is still read and composed (novox/hq issue 574). betterbird's +// builds from before mesh-catalog 8f80cd4 declare a preference and own a mergeable file that names no +// keys: refused where a definition is written, but a machine still runs that build and a rollback may +// put it back, so reading it again — ManifestAt, which a walk, the gate and a release compose from — +// must not refuse it. +func TestAStoredBuildWithASilentMergeableFileIsStillReadAndComposed(t *testing.T) { + inv := fresh(t) + ctx := context.Background() + manifest, err := json.Marshal(map[string]any{ + "module": "mail-client", "version": "1", + "settings": map[string]any{"prefs": map[string]any{ + "kind": catalogue.KindPreference, "default": "// none", "why": "its own defaults until a machine names one"}}, + "resources": []any{ + map[string]any{"id": "user-js", "type": "file", "path": "/var/lib/mail-client/user.js", "mode": "0644", + "content": "${setting:prefs}\n"}, + map[string]any{"id": "accounts", "type": "file", "path": "/var/lib/mail-client/accounts.json", + "mode": "0600", "merge": "json", "content": "{\"accounts\": {}}\n"}, + }, + }) + if err != nil { + t.Fatal(err) + } + b := aBuild("before-574", "mail-client", "") + b.Manifest = manifest + if err := inv.RecordBuild(ctx, b); err != nil { + t.Fatal(err) + } + + m, found, err := inv.ManifestAt(ctx, "mail-client", b.Commit) + if err != nil || !found { + t.Fatalf("the stored build is not read again: found %v, %v", found, err) + } + if written := catalogue.WrittenSettingsProblems(m); len(written) != 1 { + t.Fatalf("the rule this build predates should name its accounts file where it is written: %v", written) + } + + // And composed, in today's form: the accounts file still takes its layer's keys. + with := catalogue.Rendering{Settings: catalogue.SettingsBy{"mail-client": {{From: "workstation", + Values: map[string]any{"accounts": map[string]any{"work": map[string]any{"email": "a@example.invalid"}}}}}}} + if err := catalogue.JudgeSettings(m, with.Settings["mail-client"], false); err != nil { + t.Fatalf("the stored build's settings do not compose: %v", err) + } + out, err := catalogue.Resolution{Node: "workstation", Modules: []catalogue.Manifest{m}}.Declaration(with) + if err != nil { + t.Fatalf("the stored build is not composed: %v", err) + } + for _, r := range out { + if r["id"] == "mail-client.accounts" { + if content, _ := r["content"].(string); !strings.Contains(content, "a@example.invalid") { + t.Fatalf("the accounts file did not take its layer: %v", content) + } + return + } + } + t.Fatalf("the accounts file was not composed: %v", out) +} diff --git a/testdata/store-tests b/testdata/store-tests index 14d5ada0..da888096 100644 --- a/testdata/store-tests +++ b/testdata/store-tests @@ -37,3 +37,4 @@ ./cmd/mesh-controller TestReplay401 ./cmd/mesh-controller TestReplay201ThePlansSend ./internal/inventory TestAWalksRestIsReadFromEachMachinesOwnSend +./internal/inventory TestAStoredBuildWithASilentMergeableFileIsStillReadAndComposed