From 5ca888530afe81d8b99095f3342fbfe6f5a587d3 Mon Sep 17 00:00:00 2001 From: Lukas Wuttke Date: Tue, 7 Jul 2026 09:29:52 +0200 Subject: [PATCH] feat(data ingest): destination-table guard (--overwrite) + honest extension handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes for the same failure class — the customer uploads their whole dataset and only then learns it was doomed: cli#70 (P4-lite) — table-exists guard: - One cheap read (the data list query) after cluster discovery, BEFORE staging. Existing table without --overwrite → exit 6 (new, documented) with the full remedy; --overwrite replaces it via the exact teardown data delete uses. The check fails OPEN with a visible note (the in-cluster duplicate check still backstops). - Teardown acts on the MATCHED name, not the flag's casing (Linux MySQL + PVC paths are case-sensitive; acting on the flag spelling could silently no-op the DROP/rm and claim success). - --overwrite + --idempotency-key is refused outright: a replayed submit attaches to the PREVIOUS run after the teardown deleted the data — false success + data loss. (Adversarial-review catch.) - Honest partial-failure copy: a half-finished replace names `data delete` as the primary recovery — a plain re-run would pass the DB-backed guard and hit the leftover files after a full upload. - The teardown pod honors --stage-pod-image (air-gapped registries). cli#68 — extension detection/emission: - .webp removed from the accept-set: the ingestor's FileExtension enum + the ingest.v1 schema allow only .jpg/.jpeg/.png for images, and FileTypeValidator RAISES on webp — accepting it locally guaranteed an in-cluster failure after the full upload. (The old comment claiming chart support was itself the cli#68 drift.) - The single shared extension is detected, shown in the summary ("3 files (.png)"), and emitted as spec.file_options.extension so the cluster validates the type that was actually staged — previously it checked its .jpeg convention default and rejected .jpg/.png datasets after upload. - Mixed types fail locally with counts (exit 3); an all-unsupported dataset names what was found vs accepted. Cross-repo traced against data-ingestors (conventions merge, per- category validator factories, DuplicateValidator) and live-verified on a real cluster: PNG detection, guard on an existing table (exit 6), --overwrite dry-run creates nothing, mixed extensions refused, combo flag refusal. go build/vet/test green; new tests cover the guard seam (matched-name contract, fail-open), extension detection, spec emission + schema validation (keypoint top-level fields pinned), and the summary rendering. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/cli/data.go | 121 ++++++++++++++++++++++++++++++++++++- internal/cli/data_test.go | 59 ++++++++++++++++++ internal/push/detect.go | 13 ++-- internal/push/spec.go | 29 ++++++--- internal/push/spec_test.go | 65 ++++++++++++++++++++ internal/push/walk.go | 76 ++++++++++++++++++++--- internal/push/walk_test.go | 46 +++++++++++--- 7 files changed, 375 insertions(+), 34 deletions(-) diff --git a/internal/cli/data.go b/internal/cli/data.go index 076fa2fa..dad1c2d1 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "io" + "k8s.io/client-go/kubernetes" "os" "path/filepath" "strings" @@ -98,6 +99,7 @@ func newDataIngestCmd() *cobra.Command { // Operations flags. dryRun bool + overwrite bool noInput bool outputJSON bool @@ -144,7 +146,9 @@ Expected local layout (image_classification shown): 002.jpg ... -Accepted image extensions: .jpg, .jpeg, .png, .webp (case-insensitive). +Accepted image extensions: .jpg, .jpeg, or .png (case-insensitive). +All images in one dataset must share a single type — the cluster +validates the type it was told to expect. v0.1 caps the dataset at 1 GiB total + 500 MiB per file. Larger datasets need the v0.2 cloud-source story (S3/GCS/HTTPS sources) — @@ -158,8 +162,11 @@ Exit codes: 4 cluster reachable but no tracebloc client / shared storage missing 5 ingestor SA token couldn't be obtained, or jobs-manager rejected the token (401/403) + 6 destination table already exists (re-run with --overwrite to + replace it, or pick a different --table) 7 pre-flight succeeded but staging the files failed - (Pod creation, image pull, exec stream, or remote tar error) + (Pod creation, image pull, exec stream, or remote tar error) — + or, with --overwrite, removing the old table failed 8 jobs-manager rejected the submit (4xx/5xx other than auth) 9 ingestion Job exited non-zero, or completed with row-level failures the summary panel reports`, @@ -202,6 +209,7 @@ Exit codes: TargetSizeFlag: targetSize, SchemaFlag: schemaFlag, DryRun: dryRun, + Overwrite: overwrite, IngestorSAName: ingestorSAName, StagePodImage: stagePodImage, Detach: detach, @@ -251,6 +259,8 @@ Exit codes: cmd.Flags().IntVar(&numberOfKeypoints, "number-of-keypoints", 0, "keypoint_detection only: number of keypoints per sample (required; e.g. 17 for COCO pose)") + cmd.Flags().BoolVar(&overwrite, "overwrite", false, + "replace the destination table if it already exists: its current table + files are removed first (same as `tracebloc data delete`), then the new data is ingested. Not combinable with --idempotency-key") cmd.Flags().BoolVar(&dryRun, "dry-run", false, "validate + discover + walk, but don't create any cluster resources") cmd.Flags().BoolVar(&noInput, "no-input", false, @@ -291,6 +301,7 @@ type runDataIngestArgs struct { TargetSizeFlag string // raw --target-size; resolved after Discover (image) SchemaFlag string // raw --schema; resolved or inferred after Discover (tabular) DryRun bool + Overwrite bool IngestorSAName string StagePodImage string @@ -372,6 +383,16 @@ func runDataIngest(ctx context.Context, out, errOut io.Writer, a runDataIngestAr // does, so a first-time user understands it before any prompts. // Routed through a.Printer, so --output-json keeps it on stderr and // --plain/non-TTY degrade cleanly. (#31) + // --overwrite + a reused --idempotency-key is a data-loss trap: the + // teardown removes the existing data, then jobs-manager treats the + // duplicate key as a REPLAY and attaches to the previous run instead of + // ingesting anything — old data gone, new data never loaded, exit 0 from + // the old Job's status. Refuse the combination outright. + if a.Overwrite && a.IdempotencyKey != "" { + return &exitError{code: 2, err: errors.New( + "--overwrite can't be combined with --idempotency-key: a reused key makes the cluster replay the previous run instead of ingesting the new data — after --overwrite's removal that would report success while loading nothing. Drop one of the two (a fresh per-run key is the default).")} + } + a.Printer.Banner("tracebloc", "data ingest") a.Printer.Para(strings.TrimSpace(` This uploads a dataset from your machine into your tracebloc workspace so models @@ -542,6 +563,15 @@ other collaborators train against it without ever seeing the raw files.`)) "resolution mismatch.\n", derr) } } + // Extension: every image must share one type, and the spec tells + // the cluster which one to validate against (file_options.extension). + // Without this the ingestor checked its .jpeg convention default and + // rejected .jpg/.png datasets AFTER the full upload (cli#68). + ext, exterr := push.DetectExtension(layout.Images) + if exterr != nil { + return &exitError{code: 3, err: exterr} + } + a.Spec.Extension = ext default: // Text family: no extra per-category resolution. The label (for // text_classification) comes straight from --label-column; @@ -610,6 +640,28 @@ other collaborators train against it without ever seeing the raw files.`)) // before any bytes move. printClusterSummary(a.Printer, release, pvc) + // 8a. Destination guard (cli#70, P4-lite): re-ingesting an existing + // table used to stage EVERYTHING and then fail the in-cluster Job + // on the ingestor's duplicate check — a full upload burned to learn + // the table exists. One cheap read heads that off. The check fails + // open (dim note) — the ingestor still refuses duplicates, so a + // broken check can't cause silent data loss. + existingTable, checkNote := destTableExists(ctx, cs, resolved, a.Spec.Table) + if checkNote != "" { + a.Printer.Hintf("%s", checkNote) + } + tableExists := existingTable != "" + if tableExists && !a.Overwrite { + return &exitError{code: 6, err: fmt.Errorf( + "table %q already exists in this client. Re-ingesting the same table doesn't merge or replace — "+ + "the run would fail after uploading everything. Re-run with --overwrite to replace it, "+ + "or pick a different --table. (`tracebloc data delete %s` also removes it.)", + existingTable, existingTable)} + } + if tableExists && a.Overwrite { + a.Printer.Warnf("Table %q already exists — --overwrite replaces it (table + files).", existingTable) + } + // 8. Dry-run stop. Acknowledged success, plus a reminder of the // live-only steps (stage + ingest) the customer just skipped. if a.DryRun { @@ -623,6 +675,36 @@ other collaborators train against it without ever seeing the raw files.`)) return nil } + // 8b. --overwrite: remove the existing table + files before staging — + // the same teardown `data delete` runs, so the semantics match. + if tableExists && a.Overwrite { + // Tear down the MATCHED name, not the flag's spelling — table names + // are case-sensitive on Linux MySQL and PVC paths always are, so + // acting on a differently-cased --table would silently no-op the + // DROP/rm and then "succeed". + a.Printer.Infof("Removing the existing %q first…", existingTable) + plan := push.PlanTeardown(existingTable) + if _, terr := push.Teardown(ctx, cs, &push.SPDYExecutor{Config: resolved.RestConfig, Client: cs}, resolved.Namespace, plan, push.PodSpecOptions{ + Namespace: resolved.Namespace, + PVCClaimName: pvc.ClaimName, + PVCMountPath: pvc.MountPath, + Table: existingTable, + ServiceAccountName: release.IngestorSAName, + Image: a.StagePodImage, + }); terr != nil { + // The teardown drops the table before removing files, so a + // partial failure can leave files the DB-backed guard can no + // longer see — a plain re-run would upload everything and then + // hit them in-cluster. data delete first is the real recovery. + return &exitError{code: 7, err: fmt.Errorf( + "replacing table %q failed partway — its removal may be incomplete, and a plain re-run "+ + "would hit the leftovers after uploading everything. Run `tracebloc data delete %s` "+ + "first, then re-run this ingest. Nothing new was staged. (%w)", + existingTable, existingTable, terr)} + } + a.Printer.Successf("Removed the old %q — ingesting the new data.", existingTable) + } + // 9. Stage the files: create ephemeral Pod → wait Ready → tar // stream → cleanup. The deferred cleanup inside push.Stage // runs on success and failure (including ctx cancellation @@ -814,7 +896,15 @@ func printLocalSummary(p *ui.Printer, layout *push.LocalLayout, spec map[string] } default: p.Field("labels.csv", layout.LabelsCSV) - p.Field("images", fmt.Sprintf("%d files", len(layout.Images))) + imagesVal := fmt.Sprintf("%d files", len(layout.Images)) + if ext, _ := spec["spec"].(map[string]any); ext != nil { + if fo, _ := ext["file_options"].(map[string]any); fo != nil { + if e, _ := fo["extension"].(string); e != "" { + imagesVal = fmt.Sprintf("%d files (%s)", len(layout.Images), e) + } + } + } + p.Field("images", imagesVal) if anns := layout.Sidecars["annotations"]; len(anns) > 0 { p.Field("annotations", fmt.Sprintf("%d files", len(anns))) } @@ -927,3 +1017,28 @@ func writePushErrorJSON(w io.Writer, sp push.SpecArgs, e error, code int) { } _, _ = fmt.Fprintln(w, string(b)) } + +// listDatasetsFn is a test seam over push.ListDatasets. +var listDatasetsFn = push.ListDatasets + +// destTableExists reports whether the destination table already holds an +// ingested dataset, via the same query `data list` uses. It fails OPEN: a +// broken check returns (false, note) so the ingest proceeds — the in-cluster +// duplicate check still backstops it — but the note tells the user the guard +// didn't run rather than silently skipping it. +// The first return is the EXISTING table's exact name ("" when absent): +// matching is case-insensitive (mysql's catalog may be), but any teardown +// must act on the real spelling — DROP/rm against the flag's casing would +// silently no-op on case-sensitive systems and then claim success. +func destTableExists(ctx context.Context, cs kubernetes.Interface, resolved *cluster.ResolvedConfig, table string) (string, string) { + names, err := listDatasetsFn(ctx, cs, resolved.RestConfig, resolved.Namespace) + if err != nil { + return "", fmt.Sprintf("(couldn't check whether %q already exists — continuing; the cluster still refuses duplicates: %v)", table, err) + } + for _, n := range names { + if strings.EqualFold(n, table) { + return n, "" + } + } + return "", "" +} diff --git a/internal/cli/data_test.go b/internal/cli/data_test.go index cd972f05..c31c6969 100644 --- a/internal/cli/data_test.go +++ b/internal/cli/data_test.go @@ -1,7 +1,15 @@ package cli import ( + "github.com/tracebloc/cli/internal/push" + "github.com/tracebloc/cli/internal/ui" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" + "bytes" + "context" + "errors" + "github.com/tracebloc/cli/internal/cluster" "os" "path/filepath" "strings" @@ -344,3 +352,54 @@ func TestAliasResolution(t *testing.T) { }) } } + +// destTableExists backs the cli#70 P4-lite guard: an existing destination +// table must be caught BEFORE staging (a re-ingest used to burn the full +// upload and then fail in-cluster), and a broken check must fail OPEN with +// a visible note — never block the ingest, never pretend it ran. +func TestDestTableExists(t *testing.T) { + resolved := &cluster.ResolvedConfig{Namespace: "ns"} + + restore := listDatasetsFn + defer func() { listDatasetsFn = restore }() + + listDatasetsFn = func(_ context.Context, _ kubernetes.Interface, _ *rest.Config, _ string) ([]string, error) { + return []string{"other", "MyTable"}, nil + } + matched, note := destTableExists(context.Background(), nil, resolved, "mytable") + if matched != "MyTable" || note != "" { + t.Errorf("case-insensitive match must return the EXISTING spelling (teardown acts on it): matched=%q note=%q, want MyTable/empty", matched, note) + } + + matched, note = destTableExists(context.Background(), nil, resolved, "fresh_table") + if matched != "" || note != "" { + t.Errorf("absent table: matched=%q note=%q, want empty/empty", matched, note) + } + + listDatasetsFn = func(_ context.Context, _ kubernetes.Interface, _ *rest.Config, _ string) ([]string, error) { + return nil, errors.New("mysql pod not found") + } + matched, note = destTableExists(context.Background(), nil, resolved, "t") + if matched != "" { + t.Error("a broken check must fail open (no match), not closed") + } + if !strings.Contains(note, "couldn't check") || !strings.Contains(note, "mysql pod not found") { + t.Errorf("fail-open note = %q, want it to say the check didn't run and why", note) + } +} + +// The images summary line surfaces the detected extension — the visible +// half of the cli#68 fix (the spec half is pinned in internal/push). +func TestPrintLocalSummary_ShowsDetectedExtension(t *testing.T) { + var buf bytes.Buffer + p := ui.New(&buf, ui.WithColor(false)) + layout := &push.LocalLayout{Root: "/d", LabelsCSV: "/d/labels.csv", Images: []string{"/d/images/a.png"}} + spec := map[string]any{ + "table": "t", "category": "image_classification", "intent": "train", + "spec": map[string]any{"file_options": map[string]any{"extension": ".png"}}, + } + printLocalSummary(p, layout, spec) + if !strings.Contains(buf.String(), "1 files (.png)") { + t.Errorf("summary missing detected extension:\n%s", buf.String()) + } +} diff --git a/internal/push/detect.go b/internal/push/detect.go index 9a2b838e..896d45c6 100644 --- a/internal/push/detect.go +++ b/internal/push/detect.go @@ -9,9 +9,9 @@ import ( // Register the stdlib image decoders so image.DecodeConfig can // read the headers of the formats the image_classification layout - // accepts. webp is NOT in the stdlib — DetectImageSize returns an - // error for it and the caller falls back to requiring - // --target-size. (.jpg/.jpeg both decode via image/jpeg.) + // accepts (.jpg/.jpeg both decode via image/jpeg). Formats without + // a registered decoder never reach this function anymore — Discover + // skips anything outside the ingestor's accept-set (cli#68). _ "image/gif" _ "image/jpeg" _ "image/png" @@ -22,9 +22,10 @@ import ( // read the pixel data, so it's cheap even for large images). // // Supports the stdlib-registered formats (jpeg, png, gif). Returns an -// error for formats without a registered decoder (notably webp); the -// caller treats that as "couldn't auto-detect" and falls back to the -// ingestor default, advising --target-size. +// error for formats without a registered decoder; the caller treats +// that as "couldn't auto-detect" and falls back to the ingestor +// default, advising --target-size. (Since Discover only yields the +// ingestor's accept-set — .jpg/.jpeg/.png — that path is defensive.) func DetectImageSize(path string) (width, height int, err error) { f, err := os.Open(path) if err != nil { diff --git a/internal/push/spec.go b/internal/push/spec.go index a28d7e8d..98452773 100644 --- a/internal/push/spec.go +++ b/internal/push/spec.go @@ -175,6 +175,13 @@ type SpecArgs struct { // it from there, so this needs no top-level schema field. 0 ⇒ // unset (ignored for non-keypoint categories). NumberOfKeypoints int + + // Extension is the single image file extension every file in the + // dataset shares (detected by DetectExtension), emitted as + // spec.file_options.extension so the ingestor's FileTypeValidator + // checks against what was actually staged instead of its .jpeg + // convention default (cli#68). + Extension string } // Build produces the ingest.v1.json-conforming spec map. The @@ -256,6 +263,15 @@ func (a SpecArgs) buildImage(spec map[string]any, prefix string) { spec["annotations"] = path.Join(prefix, "annotations") + "/" } + // file_options carries the per-file conventions the ingestor's + // validators read: the detected extension (all categories in the + // image family — FileTypeValidator checks images against it) and, + // for the non-keypoint categories, the resolution override. + fileOptions := map[string]any{} + if a.Extension != "" { + fileOptions["extension"] = a.Extension + } + if a.Category == "keypoint_detection" { if len(a.TargetSize) == 2 { // Schema documents target_size as [height, width]; TargetSize @@ -265,16 +281,13 @@ func (a SpecArgs) buildImage(spec map[string]any, prefix string) { if a.NumberOfKeypoints > 0 { spec["number_of_keypoints"] = a.NumberOfKeypoints } - return + } else if len(a.TargetSize) == 2 { + // [height, width] per the schema; TargetSize is [W, H]. + fileOptions["target_size"] = []int{a.TargetSize[1], a.TargetSize[0]} } - if len(a.TargetSize) == 2 { - spec["spec"] = map[string]any{ - "file_options": map[string]any{ - // [height, width] per the schema; TargetSize is [W, H]. - "target_size": []int{a.TargetSize[1], a.TargetSize[0]}, - }, - } + if len(fileOptions) > 0 { + spec["spec"] = map[string]any{"file_options": fileOptions} } } diff --git a/internal/push/spec_test.go b/internal/push/spec_test.go index 09c6a4c5..83ed86c7 100644 --- a/internal/push/spec_test.go +++ b/internal/push/spec_test.go @@ -446,3 +446,68 @@ func TestStagedPrefix_PanicsOnUnsafeName(t *testing.T) { }) } } + +// The cli#68 fix: Build must carry the detected extension into +// spec.file_options so the ingestor's FileTypeValidator checks the type +// that was actually staged — and the result must still schema-validate +// (the enum only allows what the ingestor's FileExtension enum allows). +func TestBuild_EmitsDetectedExtension_PassesSchema(t *testing.T) { + for _, tc := range []struct { + name string + args SpecArgs + }{ + {"image_classification with target size", SpecArgs{ + Table: "t1", Category: "image_classification", Intent: "train", + LabelColumn: "label", TargetSize: []int{512, 512}, Extension: ".png", + }}, + {"image_classification extension only", SpecArgs{ + Table: "t2", Category: "image_classification", Intent: "train", + LabelColumn: "label", Extension: ".jpg", + }}, + {"keypoint_detection keeps top-level fields", SpecArgs{ + Table: "t3", Category: "keypoint_detection", Intent: "train", + LabelColumn: "label", TargetSize: []int{256, 256}, + NumberOfKeypoints: 17, Extension: ".jpeg", + }}, + } { + t.Run(tc.name, func(t *testing.T) { + spec := tc.args.Build() + inner, _ := spec["spec"].(map[string]any) + if inner == nil { + t.Fatal("spec.file_options missing entirely") + } + fo, _ := inner["file_options"].(map[string]any) + if fo == nil || fo["extension"] != tc.args.Extension { + t.Fatalf("file_options.extension = %v, want %q (file_options=%v)", + fo["extension"], tc.args.Extension, fo) + } + if tc.args.Category == "keypoint_detection" { + if spec["number_of_keypoints"] != 17 { + t.Errorf("keypoint top-level fields must survive: %v", spec) + } + if _, hasTS := spec["target_size"]; !hasTS { + t.Error("keypoint target_size must stay present top-level") + } + if _, hasTS := fo["target_size"]; hasTS { + t.Errorf("keypoint target_size must stay top-level, not in file_options") + } + } + + specBytes, err := yaml.Marshal(spec) + if err != nil { + t.Fatalf("marshal: %v", err) + } + v, err := schema.NewV1Validator() + if err != nil { + t.Fatalf("NewV1Validator: %v", err) + } + _, errs, parseErr := v.ValidateYAML(specBytes) + if parseErr != nil { + t.Fatalf("parse error on our own output: %v\n%s", parseErr, specBytes) + } + if len(errs) != 0 { + t.Fatalf("schema rejects the emitted extension spec: %v\n%s", errs, specBytes) + } + }) + } +} diff --git a/internal/push/walk.go b/internal/push/walk.go index 0709bcc1..98017617 100644 --- a/internal/push/walk.go +++ b/internal/push/walk.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" ) @@ -86,12 +87,15 @@ func (l *LocalLayout) FileCount() int { return n } -// imageExtensions accepts the file types the chart's -// image_classification ingestor processes by default. From -// data-ingestors' FileTypeValidator(images) defaults: .jpg, .jpeg, -// .png. The chart's defaults file (see chartversion 1.3.5+) also -// accepts .webp; we mirror that here so customers on a recent -// chart can stage webp files without hitting "no images found." +// imageExtensions accepts the file types the in-cluster ingestor can +// actually validate: data-ingestors' FileExtension enum + the +// ingest.v1 schema's file_options.extension enum both allow exactly +// .jpg, .jpeg, and .png for images. (.webp was listed here previously +// on the strength of a chart-defaults comment, but the ingestor's +// FileTypeValidator REJECTS it at construction — accepting it locally +// meant a full upload followed by a guaranteed in-cluster failure. +// cli#68 / backend#828 P5: this list and the emitted +// file_options.extension keep the two sides in lock-step.) // // Comparison is case-insensitive — filesystems vary (case-sensitive // on Linux, case-preserving-but-insensitive on macOS default APFS). @@ -99,14 +103,13 @@ var imageExtensions = map[string]struct{}{ ".jpg": {}, ".jpeg": {}, ".png": {}, - ".webp": {}, } // Discover walks rootDir and validates it matches the layout Phase 3 // expects for image_classification: // // - /labels.csv (required) -// - /images/*.{jpg,jpeg,png,webp} (at least one file) +// - /images/*.{jpg,jpeg,png} (at least one file, one type) // // Returns specific errors keyed to the layout mistakes a customer // is most likely to hit — these surface as the CLI's diagnostic @@ -190,7 +193,7 @@ func Discover(rootDir string) (*LocalLayout, error) { if errors.Is(err, os.ErrNotExist) { return nil, fmt.Errorf( "missing images/ subdirectory in %q. The CLI expects "+ - "/labels.csv + /images/*.{jpg,jpeg,png,webp}.", + "/labels.csv + /images/*.{jpg,jpeg,png}.", abs) } return nil, fmt.Errorf("stat images/: %w", err) @@ -210,6 +213,7 @@ func Discover(rootDir string) (*LocalLayout, error) { if err != nil { return nil, fmt.Errorf("reading images/: %w", err) } + skippedExts := map[string]int{} for _, entry := range entries { if entry.IsDir() { // Silently skip subdirectories so a stray .DS_Store or @@ -221,6 +225,9 @@ func Discover(rootDir string) (*LocalLayout, error) { } ext := strings.ToLower(filepath.Ext(entry.Name())) if _, ok := imageExtensions[ext]; !ok { + if ext != "" { + skippedExts[ext]++ + } continue } // entry.Info() returns Lstat-like metadata for the @@ -245,8 +252,15 @@ func Discover(rootDir string) (*LocalLayout, error) { } if len(layout.Images) == 0 { + if len(skippedExts) > 0 { + return nil, fmt.Errorf( + "no usable image files in %q — found %s, but the ingestor "+ + "accepts only .jpg, .jpeg, or .png. Convert the images "+ + "and re-run.", + imagesDir, extCounts(skippedExts)) + } return nil, fmt.Errorf( - "no image files found in %q. Expected .jpg, .jpeg, .png, or .webp; "+ + "no image files found in %q. Expected .jpg, .jpeg, or .png; "+ "got %d non-image entries.", imagesDir, len(entries)) } @@ -347,3 +361,45 @@ func HumanBytes(n int64) string { return fmt.Sprintf("%d B", n) } } + +// DetectExtension returns the single file extension (lowercased, with +// dot) shared by every image in the set. The in-cluster ingestor's +// FileTypeValidator enforces ONE extension per dataset, so a mixed set +// would pass local preflight and then fail in-cluster after the full +// upload — the exact cli#68 failure class. Detecting and emitting it +// here (spec.file_options.extension) keeps the preflight's promise: +// if this passes, the ingestor's file-type check passes too. +func DetectExtension(images []string) (string, error) { + if len(images) == 0 { + return "", fmt.Errorf("no image files to detect a type from") + } + counts := map[string]int{} + for _, img := range images { + counts[strings.ToLower(filepath.Ext(img))]++ + } + if len(counts) == 1 { + for ext := range counts { + return ext, nil + } + } + return "", fmt.Errorf( + "your images mix file types (%s) — the ingestor requires one type "+ + "per dataset. Convert them to a single type, or split them into "+ + "separate tables.", + extCounts(counts)) +} + +// extCounts renders {".png": 3, ".jpg": 120} as ".jpg ×120, .png ×3" +// (sorted for stable output). +func extCounts(counts map[string]int) string { + exts := make([]string, 0, len(counts)) + for ext := range counts { + exts = append(exts, ext) + } + sort.Strings(exts) + parts := make([]string, 0, len(exts)) + for _, ext := range exts { + parts = append(parts, fmt.Sprintf("%s ×%d", ext, counts[ext])) + } + return strings.Join(parts, ", ") +} diff --git a/internal/push/walk_test.go b/internal/push/walk_test.go index 441400c6..114cbe8f 100644 --- a/internal/push/walk_test.go +++ b/internal/push/walk_test.go @@ -72,18 +72,26 @@ func TestDiscover_TotalBytesSum(t *testing.T) { } } -func TestDiscover_AcceptsAllImageExtensions(t *testing.T) { - // Mirror the chart's FileTypeValidator(images) defaults — if a - // customer's image-set has .png + .webp, both should stage. +func TestDiscover_AcceptsIngestorExtensions(t *testing.T) { + // The accept-set mirrors what the in-cluster ingestor can actually + // validate: .jpg/.jpeg/.png (case-insensitive). .webp was accepted + // here historically on a chart-comment claim the ingestor never + // honored — staging it guaranteed an in-cluster failure after the + // full upload (cli#68), so it is now deliberately skipped. root := imgcDir(t, "a.jpg", "b.jpeg", "c.png", "d.webp", "e.JPG") got, err := Discover(root) if err != nil { t.Fatalf("Discover: %v", err) } - if len(got.Images) != 5 { - t.Errorf("len(Images) = %d, want 5 (case-insensitive); names=%v", + if len(got.Images) != 4 { + t.Errorf("len(Images) = %d, want 4 (webp skipped, case-insensitive); names=%v", len(got.Images), got.Images) } + for _, img := range got.Images { + if strings.HasSuffix(img, ".webp") { + t.Errorf("webp staged despite the ingestor rejecting it: %s", img) + } + } } func TestDiscover_SkipsNonImageFiles(t *testing.T) { @@ -161,8 +169,12 @@ func TestDiscover_NoAcceptedImageExtensions(t *testing.T) { if err == nil { t.Fatal("Discover returned nil error; expected no-images error") } - if !strings.Contains(err.Error(), "no image files") { - t.Errorf("error = %q, want it to mention no image files", err) + // The error must name what WAS found and what's accepted — the + // customer's fix is one conversion away. + for _, want := range []string{"no usable image files", ".gif", ".bmp", ".jpg, .jpeg, or .png"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error = %q, want it to mention %q", err, want) + } } } @@ -364,3 +376,23 @@ func TestHumanBytes(t *testing.T) { } } } + +// DetectExtension backs the cli#68 fix: the spec must tell the cluster the +// ONE extension the dataset actually uses, and mixed sets must fail locally +// (before upload), not in-cluster (after). +func TestDetectExtension(t *testing.T) { + ext, err := DetectExtension([]string{"a/x.jpg", "a/y.JPG", "a/z.jpg"}) + if err != nil || ext != ".jpg" { + t.Errorf("uniform: ext=%q err=%v, want .jpg/nil", ext, err) + } + + _, err = DetectExtension([]string{"a/x.jpg", "a/y.png", "a/z.jpg"}) + if err == nil { + t.Fatal("mixed extensions must error before any upload") + } + for _, want := range []string{".jpg ×2", ".png ×1", "one type"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("mixed error = %q, want it to mention %q", err, want) + } + } +}