refactor(plugin): centralize workload conversion + container cleanup

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.
This commit is contained in:
2026-06-19 16:21:54 +03:00
parent 6492944c8f
commit c8e71a0c34
9 changed files with 227 additions and 135 deletions
+35
View File
@@ -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
}
}
}
+52
View File
@@ -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,
}
}
+129
View File
@@ -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)
}
})
}
}
@@ -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.
@@ -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