From e6b3aa2222b8ff7d20b87b6f29372f050212b06c Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 29 Oct 2026 22:15:09 +0100 Subject: [PATCH] fix(34): WR-11 accept an inline plugins list in plugin:add again The comment-keeping summer.yaml edit refused a flow-style plugins list (plugins: [{id: a, module: b}]) that plugin:add used to handle with a full rewrite. The edit now replaces an inline list, over one or several lines, with a block list of its items plus the new ones and keeps every other line. A list it cannot edit in place (a tagged one, say) falls back to the full SaveManifest rewrite with a note that the comments were not kept. plugin:install gets the same behaviour. --- docs/console/scaffolding.md | 2 +- docs/plugins/marketplace.md | 2 +- internal/build/install.go | 6 +- internal/build/install_smoke_test.go | 24 ++--- internal/build/manifest.go | 124 +++++++++++++++++++++---- internal/build/manifest_append_test.go | 43 ++++++++- internal/build/scaffold.go | 6 +- 7 files changed, 168 insertions(+), 39 deletions(-) diff --git a/docs/console/scaffolding.md b/docs/console/scaffolding.md index 810647c8..ddf1a616 100644 --- a/docs/console/scaffolding.md +++ b/docs/console/scaffolding.md @@ -86,7 +86,7 @@ summer make:plugin acme.blog summer plugin:add plugins/acme/blog ``` -`summer plugin:add` then registers the local module: it adds the plugin to `summer.yaml`, adds a `require` and a local `replace` to the application's `go.mod`, and adds the directory to `go.work`. The next `summer build` compiles the plugin in. +`summer plugin:add` then registers the local module: it adds the plugin to `summer.yaml`, adds a `require` and a local `replace` to the application's `go.mod`, and adds the directory to `go.work`. The next `summer build` compiles the plugin in. The `summer.yaml` edit appends to the `plugins:` list and keeps the file's comments and blank lines; an inline list (`plugins: [{id: ..., module: ...}]`) is turned into a block list with the new entry. A list in a form it cannot edit in place is written out again with every plugin, and a note says the comments were not kept. ### Upgrading: plugins without a vendor directory diff --git a/docs/plugins/marketplace.md b/docs/plugins/marketplace.md index 7b9e2a86..0869bca6 100644 --- a/docs/plugins/marketplace.md +++ b/docs/plugins/marketplace.md @@ -104,7 +104,7 @@ summer plugin:install acme.blog --yes The command asks the registry for the plugin's releases and picks the newest one whose framework minimum the application meets. The application's framework version is the effective one: the newest tag of a local framework checkout when `go.mod` or `go.work` replaces the framework with a directory, otherwise the version `go.mod` requires. `@vX.Y.Z` pins a release, which must meet the framework minimum too. When every release needs a newer framework, the command stops and names the framework version to move to; it never changes the framework requirement itself. -The plugin is installed as a Go module: one `go get @` in the application, then an entry with its ID and package in `summer.yaml`. The edit appends to the `plugins:` list and keeps the file's comments and blank lines. Run `summer build` afterwards: it generates the import, and the new require in `go.mod` loses its `// indirect` marker the next time you run `go mod tidy`. A plugin that is already in `summer.yaml` is not installed again; move it to a newer release with `summer plugin:update`. +The plugin is installed as a Go module: one `go get @` in the application, then an entry with its ID and package in `summer.yaml`. The edit appends to the `plugins:` list and keeps the file's comments and blank lines (an inline `plugins: [...]` list becomes a block list, as with `summer plugin:add`). Run `summer build` afterwards: it generates the import, and the new require in `go.mod` loses its `// indirect` marker the next time you run `go mod tidy`. A plugin that is already in `summer.yaml` is not installed again; move it to a newer release with `summer plugin:update`. ### Required plugins diff --git a/internal/build/install.go b/internal/build/install.go index 6783cd5e..1a1792a0 100644 --- a/internal/build/install.go +++ b/internal/build/install.go @@ -733,9 +733,13 @@ func ApplyInstall(ctx context.Context, plan *InstallPlan, out io.Writer) error { return fail(err) } } - if err := appendManifestPlugins(filepath.Join(plan.AppDir, manifestName), entries); err != nil { + rewrote, err := appendManifestPlugins(filepath.Join(plan.AppDir, manifestName), entries) + if err != nil { return fail(err) } + if rewrote { + fmt.Fprintf(out, "note: %s had an inline plugins list summer could not edit in place; it was written again and its comments were not kept\n", manifestName) + } ids := make([]string, len(plan.Steps)) for i, s := range plan.Steps { ids[i] = s.ID + "@" + displayRef(s.Version) diff --git a/internal/build/install_smoke_test.go b/internal/build/install_smoke_test.go index 4ddf2e2a..16be0151 100644 --- a/internal/build/install_smoke_test.go +++ b/internal/build/install_smoke_test.go @@ -207,7 +207,7 @@ func TestInstallSmoke(t *testing.T) { t.Run("installed_old", func(t *testing.T) { app := helloCopy(t) - if err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.translate", Module: translateModule}}); err != nil { + if _, err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.translate", Module: translateModule}}); err != nil { t.Fatal(err) } if err := runGo(t.Context(), app, "mod", "edit", "-require="+translateModule+"@v0.0.9"); err != nil { @@ -521,7 +521,7 @@ func TestInstallSmoke(t *testing.T) { if left := installTemps(t, app); len(left) != 0 { t.Fatalf("failed plan left checkouts: %v", left) } - if err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.translate", Module: translateModule}}); err != nil { + if _, err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.translate", Module: translateModule}}); err != nil { t.Fatal(err) } if err := runGo(t.Context(), app, "mod", "edit", "-require="+translateModule+"@v0.1.0"); err != nil { @@ -807,7 +807,7 @@ func TestInstallSmoke(t *testing.T) { t.Fatal(err) } // acme.ghost: a module install the registry does not list. - if err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.ghost", Module: "example.com/acme/ghost"}}); err != nil { + if _, err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.ghost", Module: "example.com/acme/ghost"}}); err != nil { t.Fatal(err) } if err := runGo(t.Context(), app, "mod", "edit", "-require=example.com/acme/ghost@v0.1.0"); err != nil { @@ -920,7 +920,7 @@ func TestInstallSmoke(t *testing.T) { if err := runGo(t.Context(), app, "mod", "edit", "-require=example.com/acme/gone@v0.1.0"); err != nil { t.Fatal(err) } - if err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.gone", Module: "example.com/acme/gone"}}); err != nil { + if _, err := appendManifestPlugins(filepath.Join(app, manifestName), []Plugin{{ID: "acme.gone", Module: "example.com/acme/gone"}}); err != nil { t.Fatal(err) } gomod, _ := os.ReadFile(filepath.Join(app, "go.mod")) @@ -997,7 +997,7 @@ func TestInstallSmoke(t *testing.T) { } for _, c := range cases { write(t, path, c.in) - if err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err != nil { + if _, err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err != nil { t.Fatalf("%s: %v", c.name, err) } got, _ := os.ReadFile(path) @@ -1005,17 +1005,19 @@ func TestInstallSmoke(t *testing.T) { t.Fatalf("%s:\n got %q\nwant %q", c.name, got, c.want) } } - inline := "module: example.com/app\nbinary: app\nplugins: [{id: a.one, module: example.com/one}]\n" + // An inline list becomes a block list with the new item (WR-11). + inline := "# App.\nmodule: example.com/app\nbinary: app\nplugins: [{id: a.one, module: example.com/one}]\n" write(t, path, inline) - if err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err == nil { - t.Fatal("inline plugins list: want an error") + if rewrote, err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err != nil || rewrote { + t.Fatalf("inline plugins list: %v, rewrote %v", err, rewrote) } - if got, _ := os.ReadFile(path); string(got) != inline { - t.Fatalf("inline list was changed: %q", got) + want := "# App.\nmodule: example.com/app\nbinary: app\nplugins:\n - id: a.one\n module: example.com/one\n - id: acme.blog\n module: " + blogModule + "\n" + if got, _ := os.ReadFile(path); string(got) != want { + t.Fatalf("inline list:\n got %q\nwant %q", got, want) } dup := "module: example.com/app\nbinary: app\nplugins:\n - id: acme.blog\n module: example.com/other\n" write(t, path, dup) - if err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err == nil { + if _, err := appendManifestPlugins(path, []Plugin{{ID: "acme.blog", Module: blogModule}}); err == nil { t.Fatal("duplicate ID: want an error") } if got, _ := os.ReadFile(path); string(got) != dup { diff --git a/internal/build/manifest.go b/internal/build/manifest.go index d8d1d409..920ad4ec 100644 --- a/internal/build/manifest.go +++ b/internal/build/manifest.go @@ -1,6 +1,7 @@ package build import ( + "errors" "fmt" "os" "path/filepath" @@ -184,34 +185,52 @@ func SaveManifest(path string, m Manifest) error { // summer.yaml at path by inserting text, so comments, blank lines and the // order of the other keys stay as they are. The entries take the // indentation of the existing items; a file without a plugins key gets a -// new block at its end. The result is parsed again with LoadManifest, and -// any error restores the original bytes. -func appendManifestPlugins(path string, plugins []Plugin) error { +// new block at its end, and an inline (flow) plugins list is rewritten as +// a block list with the new entries. The result is parsed again with +// LoadManifest, and any error restores the original bytes. When an inline +// list cannot be edited in place, the whole file is written again with +// SaveManifest, as plugin:add did before (WR-11): rewrote is then true and +// the file's comments are gone. +func appendManifestPlugins(path string, plugins []Plugin) (rewrote bool, err error) { if len(plugins) == 0 { - return nil + return false, nil } for i, p := range plugins { if err := ValidatePluginID(p.ID); err != nil { - return fmt.Errorf("build: plugins[%d]: %w", i, err) + return false, fmt.Errorf("build: plugins[%d]: %w", i, err) } if err := validateModulePath(p.Module); err != nil { - return fmt.Errorf("build: plugins[%d] module: %w", i, err) + return false, fmt.Errorf("build: plugins[%d] module: %w", i, err) } } orig, err := os.ReadFile(path) if err != nil { - return fmt.Errorf("build: read %s: %w", path, err) + return false, fmt.Errorf("build: read %s: %w", path, err) } before, err := LoadManifest(path) if err != nil { - return err + return false, err + } + want := append(append([]Plugin(nil), before.Plugins...), plugins...) + // rewrite is the fallback for an inline list the text edit cannot + // handle: the whole manifest again, as SaveManifest writes it. + rewrite := func() (bool, error) { + m := before + m.Plugins = want + if err := SaveManifest(path, m); err != nil { + return false, err + } + return true, nil + } + updated, err := insertManifestPlugins(string(orig), before.Plugins, plugins) + if errors.Is(err, errInlinePlugins) { + return rewrite() } - updated, err := insertManifestPlugins(string(orig), plugins) if err != nil { - return fmt.Errorf("build: edit %s: %w", path, err) + return false, fmt.Errorf("build: edit %s: %w", path, err) } if err := os.WriteFile(path, []byte(updated), 0o644); err != nil { - return fmt.Errorf("build: write %s: %w", path, err) + return false, fmt.Errorf("build: write %s: %w", path, err) } restore := func(cause error) error { if werr := os.WriteFile(path, orig, 0o644); werr != nil { @@ -221,23 +240,28 @@ func appendManifestPlugins(path string, plugins []Plugin) error { } after, err := LoadManifest(path) if err != nil { - return restore(fmt.Errorf("build: summer.yaml edit did not parse, left unchanged: %w", err)) + return false, restore(fmt.Errorf("build: summer.yaml edit did not parse, left unchanged: %w", err)) } - want := append(append([]Plugin(nil), before.Plugins...), plugins...) if len(after.Plugins) != len(want) { - return restore(fmt.Errorf("build: summer.yaml edit gave %d plugins instead of %d, left unchanged", len(after.Plugins), len(want))) + return false, restore(fmt.Errorf("build: summer.yaml edit gave %d plugins instead of %d, left unchanged", len(after.Plugins), len(want))) } for i := range want { if after.Plugins[i] != want[i] { - return restore(fmt.Errorf("build: summer.yaml edit changed plugins[%d], left unchanged", i)) + return false, restore(fmt.Errorf("build: summer.yaml edit changed plugins[%d], left unchanged", i)) } } - return nil + return false, nil } +// errInlinePlugins reports an inline plugins list the text edit cannot +// rewrite in place. +var errInlinePlugins = errors.New("the plugins list is written inline in a form summer cannot edit in place") + // insertManifestPlugins returns src with plugins appended to its -// top-level plugins block. -func insertManifestPlugins(src string, plugins []Plugin) (string, error) { +// top-level plugins block. An inline (flow) list holding existing is +// replaced by a block list of existing and plugins; one whose end it +// cannot find gives errInlinePlugins. +func insertManifestPlugins(src string, existing, plugins []Plugin) (string, error) { lines := strings.SplitAfter(src, "\n") if n := len(lines); n > 0 && lines[n-1] == "" { lines = lines[:n-1] @@ -268,7 +292,21 @@ func insertManifestPlugins(src string, plugins []Plugin) (string, error) { switch rest { case "", "[]", "~", "null": default: - return "", fmt.Errorf("the plugins list is written inline; write it as a block list (one \"- id:\" item per plugin) and run the command again") + // An inline list such as plugins: [{id: a, module: b}]: replace + // its lines with a block list of the same items and the new ones. + end := flowEnd(lines, key, len("plugins:")) + if end < 0 { + return "", errInlinePlugins + } + all := append(append([]Plugin(nil), existing...), plugins...) + var b strings.Builder + b.WriteString(strings.Join(lines[:key], "")) + b.WriteString("plugins:\n") + for _, p := range all { + fmt.Fprintf(&b, " - id: %s\n module: %s\n", p.ID, p.Module) + } + b.WriteString(strings.Join(lines[end+1:], "")) + return b.String(), nil } // The block runs until the next line that starts a top-level key. end := key + 1 @@ -310,6 +348,54 @@ func insertManifestPlugins(src string, plugins []Plugin) (string, error) { return out + render(dash, gap, keyIndent) + strings.Join(tail, ""), nil } +// flowEnd returns the index of the line where the flow collection that +// starts at lines[key][col:] closes, or -1 when it does not close, does +// not start with "[", or has more than a comment after it. Quoted strings +// are skipped; a "#" after a blank outside quotes starts a comment. +func flowEnd(lines []string, key, col int) int { + depth, started := 0, false + var quote byte + for i := key; i < len(lines); i++ { + line := strings.TrimRight(lines[i], "\r\n") + j := 0 + if i == key { + j = col + } + for ; j < len(line); j++ { + c := line[j] + switch { + case quote != 0: + if c == quote { + quote = 0 + } + case c == '"' || c == '\'': + quote = c + case c == '#' && (j == 0 || line[j-1] == ' ' || line[j-1] == '\t'): + j = len(line) + case c == ' ' || c == '\t': + case !started && c != '[': + return -1 + case c == '[' || c == '{': + depth++ + started = true + case c == ']' || c == '}': + depth-- + if depth == 0 { + after := strings.TrimSpace(line[j+1:]) + if after != "" && !strings.HasPrefix(after, "#") { + return -1 + } + return i + } + } + } + if quote != 0 { + return -1 + } + } + return -1 +} + // stripYAMLComment drops a trailing " # comment" from a YAML value. func stripYAMLComment(s string) string { if i := strings.Index(s, " #"); i >= 0 { diff --git a/internal/build/manifest_append_test.go b/internal/build/manifest_append_test.go index 34f51d30..d4b9a1fb 100644 --- a/internal/build/manifest_append_test.go +++ b/internal/build/manifest_append_test.go @@ -51,6 +51,22 @@ func TestAppendManifestPlugins(t *testing.T) { want: "# App.\nmodule: example.test/app\nbinary: app\nplugins:\n" + " - id: acme.shop\n module: example.test/sm-shop-plugin\n - id: acme.pgsubs\n module: example.test/pg/subs\n", }, + { + name: "inline list becomes a block with the new items", + src: "# App.\nmodule: example.test/app\nbinary: app\nplugins: [{id: acme.user, module: example.test/u}, {id: acme.blog, module: \"example.test/b\"}] # core\n# Tags.\ntags: []\n", + want: "# App.\nmodule: example.test/app\nbinary: app\nplugins:\n" + + " - id: acme.user\n module: example.test/u\n - id: acme.blog\n module: example.test/b\n" + + " - id: acme.shop\n module: example.test/sm-shop-plugin\n - id: acme.pgsubs\n module: example.test/pg/subs\n" + + "# Tags.\ntags: []\n", + }, + { + name: "inline list over several lines", + src: "module: example.test/app\nbinary: app\nplugins: [\n {id: acme.user, module: example.test/u}, # first\n {id: acme.blog,\n module: example.test/b},\n]\ntags: []\n", + want: "module: example.test/app\nbinary: app\nplugins:\n" + + " - id: acme.user\n module: example.test/u\n - id: acme.blog\n module: example.test/b\n" + + " - id: acme.shop\n module: example.test/sm-shop-plugin\n - id: acme.pgsubs\n module: example.test/pg/subs\n" + + "tags: []\n", + }, { name: "empty inline list becomes a block", src: "module: example.test/app\nbinary: app\nplugins: [] # none yet\ntags: []\n", @@ -63,7 +79,7 @@ func TestAppendManifestPlugins(t *testing.T) { t.Run(c.name, func(t *testing.T) { path := filepath.Join(t.TempDir(), manifestName) write(t, path, c.src) - if err := appendManifestPlugins(path, add); err != nil { + if _, err := appendManifestPlugins(path, add); err != nil { t.Fatalf("appendManifestPlugins: %v", err) } got, _ := os.ReadFile(path) @@ -87,7 +103,6 @@ func TestAppendManifestPlugins(t *testing.T) { plugins []Plugin want string }{ - {"inline list", "module: example.test/app\nbinary: app\nplugins: [{id: acme.user, module: example.test/u}]\n", add, "inline"}, {"bad id", "module: example.test/app\nbinary: app\n", []Plugin{{ID: "Acme", Module: "example.test/x"}}, "plugins[0]"}, {"bad module", "module: example.test/app\nbinary: app\n", []Plugin{{ID: "acme.x", Module: "-x"}}, "module"}, {"broken file", "module: [\n", add, ""}, @@ -99,7 +114,7 @@ func TestAppendManifestPlugins(t *testing.T) { for _, r := range refusals { path := filepath.Join(t.TempDir(), manifestName) write(t, path, r.src) - err := appendManifestPlugins(path, r.plugins) + _, err := appendManifestPlugins(path, r.plugins) if err == nil || !strings.Contains(err.Error(), r.want) { t.Errorf("%s: err = %v, want %q", r.name, err, r.want) } @@ -109,11 +124,29 @@ func TestAppendManifestPlugins(t *testing.T) { } }) + t.Run("an inline list it cannot edit is written again", func(t *testing.T) { + // A tagged flow list: the text edit gives up and the manifest is + // written again, as plugin:add did before it kept comments. + path := filepath.Join(t.TempDir(), manifestName) + write(t, path, "# gone\nmodule: example.test/app\nbinary: app\nplugins: !!seq [{id: acme.user, module: example.test/u}]\n") + if _, err := LoadManifest(path); err != nil { + t.Fatalf("fixture does not parse: %v", err) + } + rewrote, err := appendManifestPlugins(path, add) + if err != nil || !rewrote { + t.Fatalf("rewrote %v, err %v", rewrote, err) + } + m, err := LoadManifest(path) + if err != nil || len(m.Plugins) != 3 || m.Plugins[0].ID != "acme.user" || m.Plugins[2] != add[1] { + t.Errorf("plugins = %+v, %v", m.Plugins, err) + } + }) + t.Run("nothing to add", func(t *testing.T) { path := filepath.Join(t.TempDir(), manifestName) src := "module: example.test/app\nbinary: app\n" write(t, path, src) - if err := appendManifestPlugins(path, nil); err != nil { + if _, err := appendManifestPlugins(path, nil); err != nil { t.Fatal(err) } if got, _ := os.ReadFile(path); string(got) != src { @@ -122,7 +155,7 @@ func TestAppendManifestPlugins(t *testing.T) { }) t.Run("missing file", func(t *testing.T) { - if err := appendManifestPlugins(filepath.Join(t.TempDir(), manifestName), add); err == nil { + if _, err := appendManifestPlugins(filepath.Join(t.TempDir(), manifestName), add); err == nil { t.Error("appendManifestPlugins on a missing file succeeded") } }) diff --git a/internal/build/scaffold.go b/internal/build/scaffold.go index 1872480a..dc5190bd 100644 --- a/internal/build/scaffold.go +++ b/internal/build/scaffold.go @@ -204,7 +204,11 @@ func AddPlugin(ctx context.Context, startDir, pluginDir string) error { if already { return nil } - return appendManifestPlugins(manifestPath, []Plugin{{ID: id, Module: modPath}}) + rewrote, err := appendManifestPlugins(manifestPath, []Plugin{{ID: id, Module: modPath}}) + if rewrote { + fmt.Fprintf(os.Stderr, "note: %s had an inline plugins list summer could not edit in place; it was written again and its comments were not kept\n", manifestName) + } + return err } func pluginName(id string) string {