fix: fix fallback logic for partially provided custom logos (#9842)

docs/reference/deploy/master-config-reference.rst

@@ -224,11 +224,19 @@ Optional. Specify a human-readable name for this cluster.
 Optional. Applies only to the Determined Enterprise Edition. This section contains options to
 customize the UI.
 
-``logo_path``
-=============
+``logo_paths``
+==============
+
+Specifies the paths to variations of the user-provided logo to be shown in the UI. Ensure these are
+accessible and reachable by the master service. The logo file should be a valid image format, with
+SVG recommended.
+
+Logo is defined in four variations, all need to be provided.
 
-Specifies the path to a user-provided logo to be shown in the UI. Ensure the path is accessible and
-reachable by the master service. The logo file should be a valid image format, with SVG recommended.
+-  ``dark_horizontal``: The logo to be shown in the dark theme in the horizontal layout.
+-  ``dark_vertical``: The logo to be shown in the dark theme in the vertical layout.
+-  ``light_horizontal``: The logo to be shown in the light theme in the horizontal layout.
+-  ``light_vertical``: The logo to be shown in the light theme in the vertical layout.
 
 *************************
  ``tensorboard_timeout``

master/internal/config/config_test.go

@@ -891,90 +891,53 @@ resource_manager:
 }
 
 func TestPickVariation(t *testing.T) {
+	userConfig := MediaAssetVariations{
+		LightHorizontal: "light-horizontal",
+		LightVeritical:  "light-vertical",
+		DarkHorizontal:  "dark-horizontal",
+		DarkVeritical:   "dark-vertical",
+	}
 	tests := []struct {
-		name        string
-		variations  MediaAssetVariations
 		mode        string
 		orientation string
 		expected    string
 	}{
 		{
-			name: "Light Horizontal prioritized",
-			variations: MediaAssetVariations{
-				LightHorizontal: "light-horizontal",
-				LightVeritical:  "light-vertical",
-				DarkHorizontal:  "dark-horizontal",
-				DarkVeritical:   "dark-vertical",
-			},
 			mode:        "",
 			orientation: "",
 			expected:    "light-horizontal",
 		},
 		{
-			name: "Light Vertical when Light Horizontal is empty",
-			variations: MediaAssetVariations{
-				LightHorizontal: "",
-				LightVeritical:  "light-vertical",
-				DarkHorizontal:  "dark-horizontal",
-				DarkVeritical:   "dark-vertical",
-			},
+			mode:        "",
+			orientation: "horizontal",
+			expected:    "light-horizontal",
+		},
+		{
 			mode:        "",
 			orientation: "vertical",
 			expected:    "light-vertical",
 		},
 		{
-			name: "Dark Horizontal when mode is dark",
-			variations: MediaAssetVariations{
-				LightHorizontal: "light-horizontal",
-				LightVeritical:  "light-vertical",
-				DarkHorizontal:  "dark-horizontal",
-				DarkVeritical:   "dark-vertical",
-			},
-			mode:        "dark",
+			mode:        "light",
 			orientation: "",
-			expected:    "dark-horizontal",
+			expected:    "light-horizontal",
 		},
 		{
-			name: "Dark Vertical when mode is dark and orientation is vertical",
-			variations: MediaAssetVariations{
-				LightHorizontal: "light-horizontal",
-				LightVeritical:  "light-vertical",
-				DarkHorizontal:  "dark-horizontal",
-				DarkVeritical:   "dark-vertical",
-			},
 			mode:        "dark",
-			orientation: "vertical",
-			expected:    "dark-vertical",
+			orientation: "",
+			expected:    "dark-horizontal",
 		},
 		{
-			name: "Fallback to Light Horizontal if no matches",
-			variations: MediaAssetVariations{
-				LightHorizontal: "light-horizontal",
-				LightVeritical:  "",
-				DarkHorizontal:  "",
-				DarkVeritical:   "",
-			},
 			mode:        "dark",
 			orientation: "vertical",
-			expected:    "light-horizontal",
-		},
-		{
-			name: "Empty variations fallback to empty string",
-			variations: MediaAssetVariations{
-				LightHorizontal: "",
-				LightVeritical:  "",
-				DarkHorizontal:  "",
-				DarkVeritical:   "",
-			},
-			mode:        "",
-			orientation: "",
-			expected:    "",
+			expected:    "dark-vertical",
 		},
 	}
 
 	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			result := tt.variations.PickVariation(tt.mode, tt.orientation)
+		name := fmt.Sprintf("mode=%s, orientation=%s", tt.mode, tt.orientation)
+		t.Run(name, func(t *testing.T) {
+			result := userConfig.PickVariation(tt.mode, tt.orientation)
 			if result != tt.expected {
 				t.Errorf("PickVariation(%v, %v) = %v; want %v", tt.mode, tt.orientation, result, tt.expected)
 			}

master/internal/config/ui_config.go

@@ -22,60 +22,47 @@ func (m MediaAssetVariations) PickVariation(mode, orientation string) string {
 		orientationHorizontal = "horizontal"
 		orientationVertical   = "vertical"
 	)
-	if mode == "" || mode == "light" {
-		if orientation == "" || orientation == orientationHorizontal {
-			if m.LightHorizontal != "" {
-				return m.LightHorizontal
-			}
+	switch mode {
+	case "dark":
+		switch orientation {
+		case orientationVertical:
+			return m.DarkVeritical
+		default:
+			return m.DarkHorizontal
 		}
-		if orientation == "" || orientation == orientationVertical {
-			if m.LightVeritical != "" {
-				return m.LightVeritical
-			}
-			if m.LightHorizontal != "" {
-				return m.LightHorizontal
-			}
+	default:
+		switch orientation {
+		case orientationVertical:
+			return m.LightVeritical
+		default:
+			return m.LightHorizontal
 		}
 	}
-
-	if mode == "dark" {
-		if orientation == "" || orientation == orientationHorizontal {
-			if m.DarkHorizontal != "" {
-				return m.DarkHorizontal
-			}
-		}
-		if orientation == "" || orientation == orientationVertical {
-			if m.DarkVeritical != "" {
-				return m.DarkVeritical
-			}
-			if m.DarkHorizontal != "" {
-				return m.DarkHorizontal
-			}
-		}
-	}
-
-	return m.LightHorizontal
 }
 
 // UICustomizationConfig holds the configuration for customizing the UI.
 type UICustomizationConfig struct {
-	// LogoPath is the path to variation of custom logo to use in the web UI.
-	LogoPath MediaAssetVariations `json:"logo_path"`
+	// LogoPaths is the path to variations of the custom logo to use in the web UI.
+	LogoPaths *MediaAssetVariations `json:"logo_paths"`
 }
 
 // Validate checks if the paths in UICustomizationConfig are valid filesystem paths and reachable.
 func (u UICustomizationConfig) Validate() []error {
 	var errs []error
+	if u.LogoPaths == nil {
+		return errs
+	}
 
 	paths := map[string]string{
-		"LightHorizontal": u.LogoPath.LightHorizontal,
-		"LightVeritical":  u.LogoPath.LightVeritical,
-		"DarkHorizontal":  u.LogoPath.DarkHorizontal,
-		"DarkVeritical":   u.LogoPath.DarkVeritical,
+		"LightHorizontal": u.LogoPaths.LightHorizontal,
+		"LightVeritical":  u.LogoPaths.LightVeritical,
+		"DarkHorizontal":  u.LogoPaths.DarkHorizontal,
+		"DarkVeritical":   u.LogoPaths.DarkVeritical,
 	}
 
 	for name, path := range paths {
 		if path == "" {
+			errs = append(errs, errors.New(name+" path is not set"))
 			continue
 		}
 		license.RequireLicense("UI Customization")
@@ -95,7 +82,5 @@ func (u UICustomizationConfig) Validate() []error {
 
 // HasCustomLogo returns whether the UI customization has a custom logo.
 func (u UICustomizationConfig) HasCustomLogo() bool {
-	// If one exists, we're good
-	return u.LogoPath.LightHorizontal != "" || u.LogoPath.LightVeritical != "" ||
-		u.LogoPath.DarkHorizontal != "" || u.LogoPath.DarkVeritical != ""
+	return u.LogoPaths != nil
 }

master/internal/core.go

@@ -1441,7 +1441,7 @@ func (m *Master) Run(ctx context.Context, gRPCLogInitDone chan struct{}) error {
 		if !m.config.UICustomization.HasCustomLogo() {
 			return echo.NewHTTPError(http.StatusNotFound)
 		}
-		return c.File(m.config.UICustomization.LogoPath.PickVariation(
+		return c.File(m.config.UICustomization.LogoPaths.PickVariation(
 			c.QueryParam("mode"), c.QueryParam("orientation"),
 		))
 	})