fix: Manifest-tag-fix

api/v1/weightsandbiases_conversion_overrides.go

@@ -20,6 +20,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -141,19 +142,13 @@ func mapLegacyOverrides(values map[string]interface{}, dst *appsv2.WeightsAndBia
 	return nil
 }
 
-// mapPerAppLegacyOverrides fails when a version is set but its manifest can't be
-// resolved: the manifest decides which values sections are applications, so
-// continuing would silently discard every per-application override while
-// reporting success.
-//
-// An absent version is not an error here — v1 routinely left the version to the
-// deployer channel, and there is nothing to drop when there is nothing to
-// resolve. "A version is required" is enforced on the v2 object by
-// validateWandbSpec, where the error names the field.
+// mapPerAppLegacyOverrides fails when no version is derivable, or when a
+// version's manifest can't be resolved: the manifest decides which values
+// sections are applications, so continuing would silently discard every
+// per-application override while reporting success.
 func mapPerAppLegacyOverrides(values map[string]interface{}, version, globalSize string, overrides map[string]appsv2.LegacyOverrides) error {
 	if version == "" {
-		logger.Info("no version derived from v1 values; skipping per-application legacy overrides")
-		return nil
+		return errors.New("manifest version required: no version derived from v1 values (spec.values.app.image.tag or spec.values.api.image.tag)")
 	}
 	apps, err := legacyManifestApps(version)
 	if err != nil {

api/v1/weightsandbiases_conversion_overrides_test.go

@@ -33,16 +33,32 @@ import (
 
 const testLegacyVersion = "0.83.0-test"
 
-// disableConversionManifestFetch keeps unit tests off the network; tests opt
-// in via withConversionManifest*.
-func disableConversionManifestFetch() {
+// defaultConversionManifest keeps unit tests off the network. It resolves to a
+// manifest with no applications, which is all conversion needs to succeed now
+// that a derivable version is mandatory: tests that assert per-application
+// overrides install their own via withConversionManifest*, and tests that need
+// the fetch to fail install an erroring resolver.
+func defaultConversionManifest() {
 	SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) {
-		return serverManifest.Manifest{}, errors.New("manifest fetch disabled in unit tests")
+		return serverManifest.Manifest{}, nil
 	})
 }
 
+// withFailingConversionManifest makes manifest resolution fail with err and
+// returns its call counter.
+func withFailingConversionManifest(t *testing.T, err error) *atomic.Int32 {
+	t.Helper()
+	var calls atomic.Int32
+	SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) {
+		calls.Add(1)
+		return serverManifest.Manifest{}, err
+	})
+	t.Cleanup(defaultConversionManifest)
+	return &calls
+}
+
 func TestMain(m *testing.M) {
-	disableConversionManifestFetch()
+	defaultConversionManifest()
 	os.Exit(m.Run())
 }
 
@@ -55,7 +71,7 @@ func withConversionManifest(t *testing.T, apps map[string]serverManifest.Applica
 		calls.Add(1)
 		return serverManifest.Manifest{Applications: apps}, nil
 	})
-	t.Cleanup(disableConversionManifestFetch)
+	t.Cleanup(defaultConversionManifest)
 	return &calls
 }
 
@@ -71,11 +87,20 @@ func withConversionManifestApps(t *testing.T, names ...string) *atomic.Int32 {
 }
 
 // withVersion adds the app.image.tag mapVersion reads, so per-app extraction
-// has a version to resolve the manifest with.
+// has a version to resolve the manifest with. Any other keys the fixture put
+// under app are preserved.
 func withVersion(values map[string]interface{}) map[string]interface{} {
-	values["app"] = map[string]interface{}{
-		"image": map[string]interface{}{"tag": testLegacyVersion},
+	app, ok := values["app"].(map[string]interface{})
+	if !ok {
+		app = map[string]interface{}{}
+	}
+	image, ok := app["image"].(map[string]interface{})
+	if !ok {
+		image = map[string]interface{}{}
 	}
+	image["tag"] = testLegacyVersion
+	app["image"] = image
+	values["app"] = app
 	return values
 }
 
@@ -90,13 +115,7 @@ func TestConvertTo_LegacyOverridesAbsent(t *testing.T) {
 }
 
 func TestConvertTo_LegacyOverridesGlobalEnvPrecedence(t *testing.T) {
-	// No version in values: global env must convert without any manifest fetch.
-	SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) {
-		t.Fatal("manifest must not be resolved when no version is derived")
-		return serverManifest.Manifest{}, nil
-	})
-	t.Cleanup(disableConversionManifestFetch)
-
+	withConversionManifestApps(t)
 	dst := &appsv2.WeightsAndBiases{}
 	src := newV1(map[string]interface{}{
 		"global": map[string]interface{}{
@@ -387,10 +406,7 @@ func TestConvertTo_LegacyOverridesResourcesDefaultSizeIsSmall(t *testing.T) {
 }
 
 func TestConvertTo_LegacyOverridesManifestUnavailable(t *testing.T) {
-	SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) {
-		return serverManifest.Manifest{}, errors.New("registry unreachable")
-	})
-	t.Cleanup(disableConversionManifestFetch)
+	withFailingConversionManifest(t, errors.New("registry unreachable"))
 
 	dst := &appsv2.WeightsAndBiases{}
 	src := newV1(withVersion(map[string]interface{}{
@@ -415,10 +431,7 @@ func TestConvertTo_LegacyOverridesManifestUnavailable(t *testing.T) {
 // manifest decides what counts as an application, so we can't know there was
 // nothing to map.
 func TestConvertTo_LegacyOverridesManifestUnavailableNoAppSections(t *testing.T) {
-	SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) {
-		return serverManifest.Manifest{}, errors.New("registry unreachable")
-	})
-	t.Cleanup(disableConversionManifestFetch)
+	withFailingConversionManifest(t, errors.New("registry unreachable"))
 
 	dst := &appsv2.WeightsAndBiases{}
 	src := newV1(withVersion(map[string]interface{}{
@@ -427,36 +440,65 @@ func TestConvertTo_LegacyOverridesManifestUnavailableNoAppSections(t *testing.T)
 	require.Error(t, src.ConvertTo(dst))
 }
 
-// TestConvertTo_NoVersionSkipsManifestFetch: without a version there is nothing
-// to resolve, so conversion proceeds and global env still converts.
-func TestConvertTo_NoVersionSkipsManifestFetch(t *testing.T) {
-	var calls atomic.Int32
-	SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) {
-		calls.Add(1)
-		return serverManifest.Manifest{}, errors.New("registry unreachable")
-	})
-	t.Cleanup(disableConversionManifestFetch)
+// TestConvertTo_NoVersionFailsBeforeManifestFetch: without a version the
+// manifest can't be resolved, and without the manifest there is no way to tell
+// which values sections are applications — so the write is rejected rather than
+// converted with every per-application override silently dropped.
+func TestConvertTo_NoVersionFailsBeforeManifestFetch(t *testing.T) {
+	calls := withFailingConversionManifest(t, errors.New("registry unreachable"))
 
 	dst := &appsv2.WeightsAndBiases{}
-	src := newV1(map[string]interface{}{
+	src := newV1NoVersion(map[string]interface{}{
 		"global": map[string]interface{}{
 			"env": map[string]interface{}{"HTTP_PROXY": "http://proxy"},
 		},
+		"api": map[string]interface{}{
+			"env": map[string]interface{}{"API_VAR": "1"},
+		},
 	})
-	require.NoError(t, src.ConvertTo(dst))
 
-	require.Contains(t, dst.Spec.Wandb.LegacyOverrides, appsv2.LegacyOverridesGlobalKey)
-	require.Equal(t, int32(0), calls.Load(), "no version means no manifest fetch")
+	err := src.ConvertTo(dst)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "manifest version required")
+	require.Equal(t, int32(0), calls.Load(), "the version is checked before any fetch is attempted")
+}
+
+// TestConvertTo_EmptyAppTagFailsLikeAbsentTag: mapVersion treats an empty tag as
+// no version at all, so it must be rejected the same way.
+func TestConvertTo_EmptyAppTagFailsLikeAbsentTag(t *testing.T) {
+	dst := &appsv2.WeightsAndBiases{}
+	src := newV1NoVersion(map[string]interface{}{
+		"app": map[string]interface{}{
+			"image": map[string]interface{}{"tag": ""},
+		},
+	})
+
+	err := src.ConvertTo(dst)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "manifest version required")
+}
+
+// TestConvertTo_VersionFromApiTagAloneSatisfiesRequirement: the api fallback is
+// a real version source, so an app-tag-less CR still converts.
+func TestConvertTo_VersionFromApiTagAloneSatisfiesRequirement(t *testing.T) {
+	withConversionManifestApps(t, "api")
+	dst := &appsv2.WeightsAndBiases{}
+	src := newV1NoVersion(map[string]interface{}{
+		"api": map[string]interface{}{
+			"image": map[string]interface{}{"tag": "0.79.2"},
+			"env":   map[string]interface{}{"API_VAR": "1"},
+		},
+	})
+
+	require.NoError(t, src.ConvertTo(dst))
+	require.Equal(t, "0.79.2", dst.Spec.Wandb.Version)
+	require.Equal(t, []corev1.EnvVar{{Name: "API_VAR", Value: "1"}},
+		dst.Spec.Wandb.LegacyOverrides["api"].Env)
 }
 
 func TestConvertTo_LegacyOverridesManifestFailureCooldown(t *testing.T) {
 	// The cooldown keeps repeat conversions from stalling on an unreachable registry.
-	var calls atomic.Int32
-	SetConversionManifestGetter(func(_ context.Context, _, _ string) (serverManifest.Manifest, error) {
-		calls.Add(1)
-		return serverManifest.Manifest{}, errors.New("registry unreachable")
-	})
-	t.Cleanup(disableConversionManifestFetch)
+	calls := withFailingConversionManifest(t, errors.New("registry unreachable"))
 
 	for i := 0; i < 3; i++ {
 		dst := &appsv2.WeightsAndBiases{}

api/v1/weightsandbiases_conversion_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
@@ -44,9 +45,14 @@ func withConversionReader(t *testing.T, secrets ...*corev1.Secret) {
 }
 
 // activeSpecSecret builds a `<cr-name>-spec-active`-shaped Secret with the
-// given values map JSON-encoded into data.values.
+// given values map JSON-encoded into data.values. Conversion prefers these
+// values over the CR's, so — like newV1 — they get a version unless the fixture
+// already supplies one.
 func activeSpecSecret(t *testing.T, namespace, crName string, values map[string]interface{}) *corev1.Secret {
 	t.Helper()
+	if values != nil && derivedLegacyVersion(values) == "" {
+		values = withVersion(values)
+	}
 	raw, err := json.Marshal(values)
 	require.NoError(t, err)
 	return &corev1.Secret{
@@ -58,7 +64,21 @@ func activeSpecSecret(t *testing.T, namespace, crName string, values map[string]
 	}
 }
 
+// newV1 builds a convertible v1 CR: conversion rejects values from which no
+// version can be derived, so a tag is injected unless the fixture already
+// carries one. Tests that exercise the missing-version path use newV1NoVersion.
 func newV1(values map[string]interface{}) *WeightsAndBiases {
+	if values == nil {
+		values = map[string]interface{}{}
+	}
+	if derivedLegacyVersion(values) == "" {
+		values = withVersion(values)
+	}
+	return newV1NoVersion(values)
+}
+
+// newV1NoVersion builds the CR from values verbatim, supplying nothing.
+func newV1NoVersion(values map[string]interface{}) *WeightsAndBiases {
 	return &WeightsAndBiases{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "wandb",
@@ -70,16 +90,39 @@ func newV1(values map[string]interface{}) *WeightsAndBiases {
 	}
 }
 
+// derivedLegacyVersion mirrors mapVersion: app.image.tag, then api.image.tag.
+func derivedLegacyVersion(values map[string]interface{}) string {
+	for _, section := range []string{"app", "api"} {
+		tag, found, err := unstructured.NestedString(values, section, "image", "tag")
+		if err == nil && found && tag != "" {
+			return tag
+		}
+	}
+	return ""
+}
+
+// TestConvertTo_EmptyValues: absent values short-circuit applyValueMappings
+// before the version requirement, so a CR with no values at all still converts.
 func TestConvertTo_EmptyValues(t *testing.T) {
 	dst := &appsv2.WeightsAndBiases{}
-	require.NoError(t, newV1(nil).ConvertTo(dst))
+	require.NoError(t, newV1NoVersion(nil).ConvertTo(dst))
 	require.Equal(t, "wandb", dst.Name)
 	require.Empty(t, dst.Spec.Wandb.Hostname)
 	require.Empty(t, dst.Spec.Wandb.License)
 	require.Empty(t, string(dst.Spec.Size))
 	require.NotContains(t, dst.Annotations, OIDCPendingAnnotation)
 }
 
+// TestConvertTo_EmptyValuesMapFailsWithoutVersion: a present-but-empty values map
+// does run the mappings, and yields no version, so it is rejected. The contrast
+// with TestConvertTo_EmptyValues is the nil short-circuit, not the version.
+func TestConvertTo_EmptyValuesMapFailsWithoutVersion(t *testing.T) {
+	dst := &appsv2.WeightsAndBiases{}
+	err := newV1NoVersion(map[string]interface{}{}).ConvertTo(dst)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "manifest version required")
+}
+
 func TestConvertTo_NoGlobalKey(t *testing.T) {
 	dst := &appsv2.WeightsAndBiases{}
 	src := newV1(map[string]interface{}{
@@ -217,13 +260,20 @@ func TestConvertTo_VersionEmptyAppFallsBackToApi(t *testing.T) {
 	require.Equal(t, "0.79.2", dst.Spec.Wandb.Version, "empty app tag should not consume the slot; api fallback applies")
 }
 
+// TestConvertTo_VersionAbsent: neither app nor api carries an image tag, so no
+// version can be derived and the conversion is rejected. The version is what
+// selects the server manifest, and without it the per-application overrides in
+// the values cannot be mapped.
 func TestConvertTo_VersionAbsent(t *testing.T) {
 	dst := &appsv2.WeightsAndBiases{}
-	src := newV1(map[string]interface{}{
+	src := newV1NoVersion(map[string]interface{}{
 		"global": map[string]interface{}{"host": "http://x"},
 	})
-	require.NoError(t, src.ConvertTo(dst))
-	require.Empty(t, dst.Spec.Wandb.Version)
+	err := src.ConvertTo(dst)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "manifest version required")
+	require.Contains(t, err.Error(), "spec.values.app.image.tag",
+		"the error should name the fields the deployer can set")
 }
 
 func TestConvertTo_VersionWithoutGlobal(t *testing.T) {