From c8e71a0c344ad657e4f6d10d0ce3085be49259c1 Mon Sep 17 00:00:00 2001 From: "alexei.dolgolyov" Date: Fri, 19 Jun 2026 16:21:54 +0300 Subject: [PATCH] refactor(plugin): centralize workload conversion + container cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three packages (api, reconciler, webhook) each carried a private 30-line toPluginWorkload() copy that had drifted — only the api version logged malformed public_faces JSON; the others swallowed it. Hoist the single implementation to plugin.WorkloadFromStore() (convert.go); store is already a plugin dependency so no new import edge or cycle forms. Likewise the dockerfile and static sources each had a private removeContainerByName() that disagreed (remove-all vs stop-at-first). Docker enforces unique container names, so the two were equivalent for every reachable state; converge on plugin.RemoveContainerByName() (container.go, stop-at-first) with a note on why remove-all was moot. Callers migrated; old copies removed. Adds convert_test.go pinning the field-by-field contract and JSON edge cases. --- internal/api/workload_convert.go | 41 +----- internal/reconciler/reconciler.go | 30 +--- internal/webhook/handler.go | 28 ---- internal/webhook/trigger_handler.go | 7 +- internal/workload/plugin/container.go | 35 +++++ internal/workload/plugin/convert.go | 52 +++++++ internal/workload/plugin/convert_test.go | 129 ++++++++++++++++++ .../plugin/source/dockerfile/deploy.go | 21 +-- .../workload/plugin/source/static/deploy.go | 19 +-- 9 files changed, 227 insertions(+), 135 deletions(-) create mode 100644 internal/workload/plugin/container.go create mode 100644 internal/workload/plugin/convert.go create mode 100644 internal/workload/plugin/convert_test.go diff --git a/internal/api/workload_convert.go b/internal/api/workload_convert.go index 3df7275..bfeb15a 100644 --- a/internal/api/workload_convert.go +++ b/internal/api/workload_convert.go @@ -2,48 +2,17 @@ package api import ( "encoding/json" - "log/slog" "github.com/alexei/tinyforge/internal/store" "github.com/alexei/tinyforge/internal/workload/plugin" ) -// toPluginWorkload converts a persisted store.Workload row into the value -// shape that Source / Trigger plugins consume. Lives in the api package -// (rather than store or plugin) to keep plugin's dependency graph free of -// store imports and avoid the cycle that would form otherwise. -// -// SourceConfig / TriggerConfig are passed through as raw JSON; the matching -// plugin decodes them with plugin.SourceConfigOf[T] / TriggerConfigOf[T]. -// PublicFaces is decoded eagerly because every consumer needs the parsed -// slice (proxy registration, UI, validation). +// toPluginWorkload is a local alias for the shared plugin.WorkloadFromStore +// converter, kept so the api package's many call sites read tersely and pair +// visually with fromPluginWorkload below. The conversion logic lives in the +// plugin package (the single home shared with reconciler / webhook). func toPluginWorkload(w store.Workload) plugin.Workload { - var faces []plugin.PublicFace - if w.PublicFaces != "" { - if err := json.Unmarshal([]byte(w.PublicFaces), &faces); err != nil { - slog.Warn("workload: invalid public_faces JSON, treating as empty", - "workload", w.ID, "error", err) - faces = nil - } - } - return plugin.Workload{ - ID: w.ID, - Name: w.Name, - GroupID: w.AppID, - ParentWorkloadID: w.ParentWorkloadID, - SourceKind: w.SourceKind, - SourceConfig: json.RawMessage(w.SourceConfig), - TriggerKind: w.TriggerKind, - TriggerConfig: json.RawMessage(w.TriggerConfig), - PublicFaces: faces, - NotificationURL: w.NotificationURL, - NotificationSecret: w.NotificationSecret, - WebhookSecret: w.WebhookSecret, - WebhookSigningSecret: w.WebhookSigningSecret, - WebhookRequireSignature: w.WebhookRequireSignature, - CreatedAt: w.CreatedAt, - UpdatedAt: w.UpdatedAt, - } + return plugin.WorkloadFromStore(w) } // fromPluginWorkload is the symmetric direction — used by /api/workloads diff --git a/internal/reconciler/reconciler.go b/internal/reconciler/reconciler.go index eab595b..ea4830a 100644 --- a/internal/reconciler/reconciler.go +++ b/internal/reconciler/reconciler.go @@ -15,7 +15,6 @@ package reconciler import ( "context" - "encoding/json" "errors" "fmt" "log/slog" @@ -169,7 +168,7 @@ func (r *Reconciler) reconcilePluginWorkloads(ctx context.Context, rows []store. if w.SourceKind == "" { continue } - pw := toPluginWorkload(w) + pw := plugin.WorkloadFromStore(w) if err := r.plugins.DispatchReconcile(ctx, pw); err != nil { slog.Warn("reconciler: plugin reconcile failed", "workload", w.ID, "kind", w.SourceKind, "error", err) @@ -177,33 +176,6 @@ func (r *Reconciler) reconcilePluginWorkloads(ctx context.Context, rows []store. } } -// toPluginWorkload mirrors the api / webhook converters; kept local to -// avoid an import dependency between those packages. -func toPluginWorkload(w store.Workload) plugin.Workload { - var faces []plugin.PublicFace - if w.PublicFaces != "" { - _ = json.Unmarshal([]byte(w.PublicFaces), &faces) - } - return plugin.Workload{ - ID: w.ID, - Name: w.Name, - GroupID: w.AppID, - ParentWorkloadID: w.ParentWorkloadID, - SourceKind: w.SourceKind, - SourceConfig: json.RawMessage(w.SourceConfig), - TriggerKind: w.TriggerKind, - TriggerConfig: json.RawMessage(w.TriggerConfig), - PublicFaces: faces, - NotificationURL: w.NotificationURL, - NotificationSecret: w.NotificationSecret, - WebhookSecret: w.WebhookSecret, - WebhookSigningSecret: w.WebhookSigningSecret, - WebhookRequireSignature: w.WebhookRequireSignature, - CreatedAt: w.CreatedAt, - UpdatedAt: w.UpdatedAt, - } -} - func (r *Reconciler) loop(ctx context.Context) { defer r.wg.Done() diff --git a/internal/webhook/handler.go b/internal/webhook/handler.go index 4c6eb0b..f75e5be 100644 --- a/internal/webhook/handler.go +++ b/internal/webhook/handler.go @@ -338,31 +338,3 @@ func buildInboundEvent(body []byte, headers http.Header) (plugin.InboundEvent, e gitEvt.Headers = headers return gitEvt, nil } - -// toPluginWorkload mirrors the api-layer converter but kept local so the -// webhook package does not depend on internal/api. Inlining is cheap and -// avoids elevating that converter to a shared package. -func toPluginWorkload(w store.Workload) plugin.Workload { - var faces []plugin.PublicFace - if w.PublicFaces != "" { - _ = json.Unmarshal([]byte(w.PublicFaces), &faces) - } - return plugin.Workload{ - ID: w.ID, - Name: w.Name, - GroupID: w.AppID, - ParentWorkloadID: w.ParentWorkloadID, - SourceKind: w.SourceKind, - SourceConfig: json.RawMessage(w.SourceConfig), - TriggerKind: w.TriggerKind, - TriggerConfig: json.RawMessage(w.TriggerConfig), - PublicFaces: faces, - NotificationURL: w.NotificationURL, - NotificationSecret: w.NotificationSecret, - WebhookSecret: w.WebhookSecret, - WebhookSigningSecret: w.WebhookSigningSecret, - WebhookRequireSignature: w.WebhookRequireSignature, - CreatedAt: w.CreatedAt, - UpdatedAt: w.UpdatedAt, - } -} diff --git a/internal/webhook/trigger_handler.go b/internal/webhook/trigger_handler.go index 186dbba..b4224ee 100644 --- a/internal/webhook/trigger_handler.go +++ b/internal/webhook/trigger_handler.go @@ -318,7 +318,7 @@ func (h *Handler) fireBinding( b store.WorkloadTriggerBinding, evt plugin.InboundEvent, ) (bool, string) { - pwl := toPluginWorkload(row) + pwl := plugin.WorkloadFromStore(row) pwl, err := plugin.WithEffectiveTrigger(pwl, trg.Kind, json.RawMessage(trg.Config), json.RawMessage(b.BindingConfig)) if err != nil { @@ -395,7 +395,7 @@ func (h *Handler) handlePreviewIntent( // it isn't bucketed as "no binding matched". return false, ReasonPreviewNoop } - childPwl := toPluginWorkload(child) + childPwl := plugin.WorkloadFromStore(child) if err := h.plugins.DispatchTeardown(ctx, childPwl); err != nil { slog.Warn("webhook: preview teardown dispatch failed", "template", template.Name, "preview", child.Name, "error", err) @@ -421,7 +421,7 @@ func (h *Handler) handlePreviewIntent( "template", template.Name, "branch", branch, "error", err) return false, ReasonPreviewError } - childPwl := toPluginWorkload(child) + childPwl := plugin.WorkloadFromStore(child) if err := h.plugins.DispatchPlugin(ctx, childPwl, *intent); err != nil { slog.Warn("webhook: preview dispatch failed", "template", template.Name, "preview", child.Name, "error", err) @@ -431,4 +431,3 @@ func (h *Handler) handlePreviewIntent( "template", template.Name, "branch", branch, "preview", child.Name, "reason", intent.Reason) return true, intent.Reason } - diff --git a/internal/workload/plugin/container.go b/internal/workload/plugin/container.go new file mode 100644 index 0000000..c954898 --- /dev/null +++ b/internal/workload/plugin/container.go @@ -0,0 +1,35 @@ +package plugin + +import "context" + +// RemoveContainerByName is a best-effort cleanup of a name conflict before a +// CreateContainer retry: enumerate the managed containers and stop+remove the +// one whose name matches. Docker enforces unique container names per daemon, +// so at most one managed container can match — the loop returns after the +// first hit. +// +// Shared by the source plugins (dockerfile, static) that previously each kept +// their own copy. Those copies had diverged: the dockerfile copy removed every +// match (defending against a since-debunked "a partial deploy can leave more +// than one matching artifact" case), while the static copy stopped at the +// first. We deliberately converge on stop-at-first: ManagedContainer.Name is +// the primary Docker name (Names[0]), which the daemon keeps globally unique +// across every container state, so the remove-all loop could never actually +// match more than one container — the two behaviours are equivalent for every +// reachable state. +// +// Failures are intentionally swallowed: the caller treats this as opportunistic +// and re-attempts CreateContainer regardless. +func RemoveContainerByName(ctx context.Context, deps Deps, name string) { + containers, err := deps.Docker.ListContainers(ctx, nil) + if err != nil { + return + } + for _, c := range containers { + if c.Name == name { + deps.Docker.StopContainer(ctx, c.ID, 10) + deps.Docker.RemoveContainer(ctx, c.ID, true) + return + } + } +} diff --git a/internal/workload/plugin/convert.go b/internal/workload/plugin/convert.go new file mode 100644 index 0000000..73bd421 --- /dev/null +++ b/internal/workload/plugin/convert.go @@ -0,0 +1,52 @@ +package plugin + +import ( + "encoding/json" + "log/slog" + + "github.com/alexei/tinyforge/internal/store" +) + +// WorkloadFromStore converts a persisted store.Workload row into the value +// shape that Source / Trigger plugins consume. It is the single converter +// shared by every caller (api, reconciler, webhook) — previously each kept +// its own byte-identical copy, which drifted (only the api copy logged bad +// PublicFaces JSON; the others swallowed it). +// +// Living in the plugin package is safe: plugin already imports store (Deps +// holds a *store.Store), so this adds no new edge to the dependency graph +// and store does not import plugin. +// +// SourceConfig / TriggerConfig are passed through as raw JSON; the matching +// plugin decodes them with plugin.SourceConfigOf[T] / TriggerConfigOf[T]. +// PublicFaces is decoded eagerly because every consumer needs the parsed +// slice (proxy registration, UI, validation); invalid JSON is logged and +// treated as empty rather than failing the conversion. +func WorkloadFromStore(w store.Workload) Workload { + var faces []PublicFace + if w.PublicFaces != "" { + if err := json.Unmarshal([]byte(w.PublicFaces), &faces); err != nil { + slog.Warn("workload: invalid public_faces JSON, treating as empty", + "workload", w.ID, "error", err) + faces = nil + } + } + return Workload{ + ID: w.ID, + Name: w.Name, + GroupID: w.AppID, + ParentWorkloadID: w.ParentWorkloadID, + SourceKind: w.SourceKind, + SourceConfig: json.RawMessage(w.SourceConfig), + TriggerKind: w.TriggerKind, + TriggerConfig: json.RawMessage(w.TriggerConfig), + PublicFaces: faces, + NotificationURL: w.NotificationURL, + NotificationSecret: w.NotificationSecret, + WebhookSecret: w.WebhookSecret, + WebhookSigningSecret: w.WebhookSigningSecret, + WebhookRequireSignature: w.WebhookRequireSignature, + CreatedAt: w.CreatedAt, + UpdatedAt: w.UpdatedAt, + } +} diff --git a/internal/workload/plugin/convert_test.go b/internal/workload/plugin/convert_test.go new file mode 100644 index 0000000..dd060a4 --- /dev/null +++ b/internal/workload/plugin/convert_test.go @@ -0,0 +1,129 @@ +package plugin + +import ( + "encoding/json" + "testing" + + "github.com/alexei/tinyforge/internal/store" +) + +// TestWorkloadFromStore_MapsEveryField pins the full field-for-field contract +// of the consolidated converter. The chief risk of extracting the three former +// per-package copies into one shared function is a silently dropped field — +// especially the three secrets (json:"-", so serialization-based tests can +// never catch a regression here) and the GroupID<-AppID rename. +func TestWorkloadFromStore_MapsEveryField(t *testing.T) { + faces := []PublicFace{ + {Subdomain: "app", Domain: "example.com", TargetService: "web", TargetPort: 8080, AccessListID: 3, EnableSSL: true}, + {Subdomain: "api", Domain: "example.com", TargetPort: 9090}, + } + facesJSON, err := json.Marshal(faces) + if err != nil { + t.Fatalf("marshal faces: %v", err) + } + + src := store.Workload{ + ID: "wl-1", + Name: "my-workload", + AppID: "grp-7", + SourceKind: "dockerfile", + SourceConfig: `{"repo":"x"}`, + TriggerKind: "git", + TriggerConfig: `{"branch":"main"}`, + PublicFaces: string(facesJSON), + ParentWorkloadID: "parent-2", + NotificationURL: "https://hooks.example.com/notify", + NotificationSecret: "notif-secret", + WebhookSecret: "wh-secret", + WebhookSigningSecret: "wh-signing-secret", + WebhookRequireSignature: true, + CreatedAt: "2026-01-01T00:00:00Z", + UpdatedAt: "2026-01-02T00:00:00Z", + } + + got := WorkloadFromStore(src) + + if got.ID != src.ID { + t.Errorf("ID = %q, want %q", got.ID, src.ID) + } + if got.Name != src.Name { + t.Errorf("Name = %q, want %q", got.Name, src.Name) + } + if got.GroupID != src.AppID { + t.Errorf("GroupID = %q, want AppID %q", got.GroupID, src.AppID) + } + if got.ParentWorkloadID != src.ParentWorkloadID { + t.Errorf("ParentWorkloadID = %q, want %q", got.ParentWorkloadID, src.ParentWorkloadID) + } + if got.SourceKind != src.SourceKind { + t.Errorf("SourceKind = %q, want %q", got.SourceKind, src.SourceKind) + } + if string(got.SourceConfig) != src.SourceConfig { + t.Errorf("SourceConfig = %q, want %q", string(got.SourceConfig), src.SourceConfig) + } + if got.TriggerKind != src.TriggerKind { + t.Errorf("TriggerKind = %q, want %q", got.TriggerKind, src.TriggerKind) + } + if string(got.TriggerConfig) != src.TriggerConfig { + t.Errorf("TriggerConfig = %q, want %q", string(got.TriggerConfig), src.TriggerConfig) + } + if got.NotificationURL != src.NotificationURL { + t.Errorf("NotificationURL = %q, want %q", got.NotificationURL, src.NotificationURL) + } + if got.NotificationSecret != src.NotificationSecret { + t.Errorf("NotificationSecret not carried through") + } + if got.WebhookSecret != src.WebhookSecret { + t.Errorf("WebhookSecret not carried through") + } + if got.WebhookSigningSecret != src.WebhookSigningSecret { + t.Errorf("WebhookSigningSecret not carried through") + } + if got.WebhookRequireSignature != src.WebhookRequireSignature { + t.Errorf("WebhookRequireSignature = %v, want %v", got.WebhookRequireSignature, src.WebhookRequireSignature) + } + if got.CreatedAt != src.CreatedAt { + t.Errorf("CreatedAt = %q, want %q", got.CreatedAt, src.CreatedAt) + } + if got.UpdatedAt != src.UpdatedAt { + t.Errorf("UpdatedAt = %q, want %q", got.UpdatedAt, src.UpdatedAt) + } + + if len(got.PublicFaces) != len(faces) { + t.Fatalf("PublicFaces len = %d, want %d", len(got.PublicFaces), len(faces)) + } + if got.PublicFaces[0] != faces[0] || got.PublicFaces[1] != faces[1] { + t.Errorf("PublicFaces = %+v, want %+v", got.PublicFaces, faces) + } +} + +// TestWorkloadFromStore_PublicFaces covers the PublicFaces decode branch, +// including the malformed-JSON path that the consolidation newly unified onto +// "log and treat as empty" for every caller (the old reconciler/webhook copies +// silently swallowed the error). A decode failure must never fail the +// conversion or panic — it yields nil faces. +func TestWorkloadFromStore_PublicFaces(t *testing.T) { + tests := []struct { + name string + raw string + wantLen int + wantNil bool + }{ + {name: "empty string yields nil", raw: "", wantLen: 0, wantNil: true}, + {name: "empty array yields empty", raw: "[]", wantLen: 0, wantNil: false}, + {name: "malformed json yields nil", raw: "{not-json", wantLen: 0, wantNil: true}, + {name: "wrong-shape json yields nil", raw: `{"a":1}`, wantLen: 0, wantNil: true}, + {name: "single valid face", raw: `[{"Subdomain":"a"}]`, wantLen: 1, wantNil: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := WorkloadFromStore(store.Workload{ID: "wl", PublicFaces: tt.raw}) + if len(got.PublicFaces) != tt.wantLen { + t.Errorf("PublicFaces len = %d, want %d", len(got.PublicFaces), tt.wantLen) + } + if tt.wantNil && got.PublicFaces != nil { + t.Errorf("PublicFaces = %+v, want nil", got.PublicFaces) + } + }) + } +} diff --git a/internal/workload/plugin/source/dockerfile/deploy.go b/internal/workload/plugin/source/dockerfile/deploy.go index 4b01505..6cf4d25 100644 --- a/internal/workload/plugin/source/dockerfile/deploy.go +++ b/internal/workload/plugin/source/dockerfile/deploy.go @@ -260,7 +260,7 @@ func deploy(ctx context.Context, deps plugin.Deps, w plugin.Workload, intent plu deps.Docker.StopContainer(ctx, prevContainerID, 10) deps.Docker.RemoveContainer(ctx, prevContainerID, true) } - removeContainerByName(ctx, deps, containerName) + plugin.RemoveContainerByName(ctx, deps, containerName) containerID, err = deps.Docker.CreateContainer(ctx, cc) if err != nil { @@ -517,25 +517,6 @@ func healUnchanged(deps plugin.Deps, w plugin.Workload, prev runtimeState, lates return nil } -// removeContainerByName enumerates Docker's view and best-effort drops -// EVERY matching container so a name conflict in CreateContainer is -// recoverable. Container names are unique per daemon, but the recovery -// path exists precisely because a conflict occurred — a prior partial -// deploy can leave more than one matching artifact, so we must not stop -// at the first. Mirrors the static plugin's helper of the same name. -func removeContainerByName(ctx context.Context, deps plugin.Deps, name string) { - containers, err := deps.Docker.ListContainers(ctx, nil) - if err != nil { - return - } - for _, c := range containers { - if c.Name == name { - deps.Docker.StopContainer(ctx, c.ID, 10) - deps.Docker.RemoveContainer(ctx, c.ID, true) - } - } -} - // primaryDomain mirrors the static plugin's helper of the same name — // derives an FQDN from the workload's first enabled public face, with // the same bare-subdomain + settings.Domain fall-through. diff --git a/internal/workload/plugin/source/static/deploy.go b/internal/workload/plugin/source/static/deploy.go index 44ed877..06cbf8d 100644 --- a/internal/workload/plugin/source/static/deploy.go +++ b/internal/workload/plugin/source/static/deploy.go @@ -289,7 +289,7 @@ func deploy(ctx context.Context, deps plugin.Deps, w plugin.Workload, intent plu deps.Docker.StopContainer(ctx, prevContainerID, 10) deps.Docker.RemoveContainer(ctx, prevContainerID, true) } - removeContainerByName(ctx, deps, containerName) + plugin.RemoveContainerByName(ctx, deps, containerName) containerID, err = deps.Docker.CreateContainer(ctx, cc) if err != nil { @@ -517,23 +517,6 @@ func publishEvent(deps plugin.Deps, w plugin.Workload, status string) { plugin.EmitDeployEvent(deps, w, "static_site", status) } -// removeContainerByName mirrors the legacy helper: enumerate Docker's -// view and best-effort drop the matching container so a name conflict -// in CreateContainer is recoverable. Best-effort. -func removeContainerByName(ctx context.Context, deps plugin.Deps, name string) { - containers, err := deps.Docker.ListContainers(ctx, nil) - if err != nil { - return - } - for _, c := range containers { - if c.Name == name { - deps.Docker.StopContainer(ctx, c.ID, 10) - deps.Docker.RemoveContainer(ctx, c.ID, true) - return - } - } -} - // primaryDomain derives the public-facing FQDN from the workload's // first enabled public face. Static workloads support at most one // face today, but iterate defensively in case the API contract