OCPBUGS-53412 CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 #2177

cajieh · 2025-01-28T22:23:57Z

The change is to add a new CustomLogos field for specifying the console logos for light and dark themes, and to mark the current CustomLogoFile field as deprecated.

cc: @jhadvig @spadgett

Example of how the CustomLogos map to the ConfigMap:

CustomLogo ConfigMap:

`oc create configmap custom-logos-config-name \
   --namespace=openshift-config \
   --from-file /path/to/<filename>

CustomLogos sample structure:

{
  "logos": [
    {
      "type": "Masthead",
      "themes": [
        {
          "mode": "Dark",
          "source": {
            "from": "ConfigMap",
            "configMap": {
              "name": "custom-logos-config",
              "key": "masthead-dark-logo.svg"
            }
          }
        },
        {
          "mode": "Light",
          "source":  {
            "from": "ConfigMap",
            "configMap": {
              "name": "custom-logos-config",
              "key": "masthead-light-logo.svg"
            }
        }
      ]
    },
    {
      "type": "Favicon",
      "themes": [
        {
          "mode": "Dark",
          "source": {
            "from": "ConfigMap",
            "configMap": {
              "name": "custom-logos-config",
              "key": "favicon-dark-logo.png"
            }
          }
        },
        {
          "mode": "Light",
          "source": {
            "from": "ConfigMap",
            "configMap": {
              "name": "custom-logos-config",
              "key": "favicon-light-logo.png"
            }
          }
        }
      ]
    }
  ]
}

openshift-ci-robot · 2025-01-28T22:24:01Z

@cajieh: This pull request references CONSOLE-4404 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

…sole]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-01-28T22:24:08Z

Hello @cajieh! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

openshift-ci-robot · 2025-01-31T11:02:48Z

@cajieh: This pull request references CONSOLE-4404 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

The change is to add a new CustomLogoFiles field for specifying the console logo for light and dark themes, and to mark the current CustomLogoFile field as deprecated.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2025-01-31T11:07:20Z

@cajieh: This pull request references CONSOLE-4448 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

The change is to add a new CustomLogoFiles field for specifying the console logo for light and dark themes, and to mark the current CustomLogoFile field as deprecated.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2025-01-31T11:10:03Z

@cajieh: This pull request references CONSOLE-4448 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

The change is to add a new CustomLogoFiles field for specifying the console logo for light and dark themes, and to mark the current CustomLogoFile field as deprecated.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2025-01-31T18:23:20Z

@cajieh: This pull request references CONSOLE-4448 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

The change is to add a new CustomLogoFiles field for specifying the console logo for light and dark themes, and to mark the current CustomLogoFile field as deprecated.

cc: @jhadvig @spadgett

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

everettraven · 2025-01-31T19:35:29Z

operator/v1/types_console.go

+// CustomFiles defines a configuration based on theme types for the console UI logo.
+type CustomFiles struct {


A couple things here:

the name CustomFiles is not something that I would generally associate with a configuration for a logo based on themes. Is there another name we could use for the struct that is more representative of this being a theme-based logo substitution (or at the least a logo substitution)?

This is pluralized but represents a single object. I would avoid pluralization of a type unless it is aliased to something like a slice or map.

everettraven · 2025-01-31T19:39:14Z

operator/v1/types_console.go

+
+// CustomFiles defines a configuration based on theme types for the console UI logo.
+type CustomFiles struct {
+	// type of the theme for the console UI.


The documentation on this field is pretty sparse. I'd recommend taking a look at https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#write-user-readable-documentation-in-godoc and thinking about how you could improve the godoc here to answer some of the key questions in the doc I linked.

everettraven · 2025-01-31T19:41:50Z

operator/v1/types_console.go

+// CustomFiles defines a configuration based on theme types for the console UI logo.
+type CustomFiles struct {
+	// type of the theme for the console UI.
+	// +unionDiscriminator


This is marked as a union discriminator, but this doesn't really seem to be implementing a discriminated union.

I don't really see a need for this to be a discriminated union so I'd recommend removing this marker.

everettraven · 2025-01-31T19:47:57Z

operator/v1/types_console.go

+type CustomFiles struct {
+	// type of the theme for the console UI.
+	// +unionDiscriminator
+	// +kubebuilder:validation:Enum:="Dark";"Light"


This is currently limited to either Dark or Light. What if a user wants to use the same logo on both dark and light themes?

This is currently limited to either Dark or Light. What if a user wants to use the same logo on both dark and light themes?

I think in that case, the user would reference the same logo for both themes. I can double check on this.

everettraven · 2025-01-31T19:48:21Z

operator/v1/types_console.go

+	// +kubebuilder:validation:Enum:="Dark";"Light"
+	// +required
+	Type ThemeType `json:"type"`
+	// path is the reference to the ConfigMap in the openshift-config namespace.


My previous comment on the sparse field documentation applies here as well.

everettraven · 2025-01-31T19:58:30Z

operator/v1/types_console.go

+	// Dimensions: Max height of 68px and max width of 200px
+	// SVG format preferred
+	// +optional
+	CustomLogoFiles CustomFiles `json:"customLogoFiles,omitempty"`


This seems like it is supposed to be a field where multiple things can be referenced, but the type used is a single object. Should this be a slice of CustomFiles ([]CustomFiles)?

This seems like it is supposed to be a field where multiple things can be referenced, but the type used is a single object. Should this be a slice of CustomFiles ([]CustomFiles)?

Right, but just thinking if this should be a single struct instead since there are only two themes, making it easier to manage and access. Something like this:

type CustomLogoThemeFiles struct { // Dark specifies the ConfigMap reference for the dark theme logo. Dark configv1.ConfigMapFileReference `json:"dark,omitempty"` // Light specifies the ConfigMap reference for the light theme logo. Light configv1.ConfigMapFileReference `json:"light,omitempty"` }

However, if more themes are added in the future, the struct will need to be updated. With a slice of structs, there will still be an update needed, at least for the enum type. WDYT?

It sounds reasonable to have a single struct, but some things I would take into consideration when transitioning to a single struct:

What constraints do you expect to be enforced? For example, can I specify only a dark theme or do I have to specify both? How do these constraints evolve as new fields are added?

How often do you anticipate new fields/options needing to be added?

JoelSpeed · 2025-03-20T11:30:56Z

Changes look good

One nit though, why are we not gating this addition? All new features are supposed to be behind feature gates now and show sufficient stability prior to being enabled by default

How complete is the rest of the implementation of this feature? Are there tests being written?

cajieh · 2025-03-20T13:25:23Z

operator/v1/tests/consoles.operator.openshift.io/AAA_ungated.yaml

+                      configMap:
+                        name: custom-logos-config
+                        key: masthead-dark-logo.svg
+      expectedError: "spec.customization.logos[0].themes[0].source.from: Unsupported value: \"InvalidSource\": supported values: \"ConfigMap\""


I think it looks right again. This trips me up.:)

cajieh · 2025-03-20T13:51:12Z

Changes look good

One nit though, why are we not gating this addition? All new features are supposed to be behind feature gates now and show sufficient stability prior to being enabled by default

How complete is the rest of the implementation of this feature? Are there tests being written?

I will defer to @jhadvig on this.

jhadvig · 2025-03-20T14:50:31Z

@JoelSpeed so this story is actually a bug in the console, which we discovered when upgrading to PatternFly version 6. PatternFly is a design system that provides a set of reusable UI components. With this update we need to provide two logos, one for dark theme and light theme. The same goes for the favicon. So technically its a bug on our end, which we need to address with the PF6 upgrade - part of 4.19.
That said we already have console and console-operators changes in place ad have been developed in parallel.

jhadvig · 2025-03-20T16:45:32Z

/retitle OCPBUGS-53412 CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6

openshift-ci-robot · 2025-03-20T16:45:42Z

@cajieh: This pull request references Jira Issue OCPBUGS-53412, which is invalid:

expected the bug to target the "4.19.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

This pull request references CONSOLE-4448 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

The change is to add a new CustomLogos field for specifying the console logos for light and dark themes, and to mark the current CustomLogoFile field as deprecated.

cc: @jhadvig @spadgett

Example of how the CustomLogos map to the ConfigMap:

CustomLogo ConfigMap:

`oc create configmap custom-logos-config-name \
  --namespace=openshift-config \
  --from-file /path/to/<filename>

CustomLogos sample structure:

{
 "logos": [
   {
     "type": "Masthead",
     "themes": [
       {
         "mode": "Dark",
         "source": {
           "from": "ConfigMap",
           "configMap": {
             "name": "custom-logos-config",
             "key": "masthead-dark-logo.svg"
           }
         }
       },
       {
         "mode": "Light",
         "source":  {
           "from": "ConfigMap",
           "configMap": {
             "name": "custom-logos-config",
             "key": "masthead-light-logo.svg"
           }
       }
     ]
   },
   {
     "type": "Favicon",
     "themes": [
       {
         "mode": "Dark",
         "source": {
           "from": "ConfigMap",
           "configMap": {
             "name": "custom-logos-config",
             "key": "favicon-dark-logo.png"
           }
         }
       },
       {
         "mode": "Light",
         "source": {
           "from": "ConfigMap",
           "configMap": {
             "name": "custom-logos-config",
             "key": "favicon-light-logo.png"
           }
         }
       }
     ]
   }
 ]
}

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

jhadvig · 2025-03-20T16:46:38Z

/jira refresh

openshift-ci-robot · 2025-03-20T16:46:48Z

@jhadvig: This pull request references Jira Issue OCPBUGS-53412, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.19.0) matches configured target version for branch (4.19.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @yapei

This pull request references CONSOLE-4448 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

JoelSpeed

/lgtm

openshift-ci · 2025-03-20T17:04:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cajieh, everettraven, jhadvig, JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2025-03-20T17:07:31Z

@cajieh: Jira Issue OCPBUGS-53412: All pull requests linked via external trackers have merged:

openshift/api#2177

Jira Issue OCPBUGS-53412 has been moved to the MODIFIED state.

In response to this:

The change is to add a new CustomLogos field for specifying the console logos for light and dark themes, and to mark the current CustomLogoFile field as deprecated.

cc: @jhadvig @spadgett

Example of how the CustomLogos map to the ConfigMap:

CustomLogo ConfigMap:

`oc create configmap custom-logos-config-name \
  --namespace=openshift-config \
  --from-file /path/to/<filename>

CustomLogos sample structure:

{
 "logos": [
   {
     "type": "Masthead",
     "themes": [
       {
         "mode": "Dark",
         "source": {
           "from": "ConfigMap",
           "configMap": {
             "name": "custom-logos-config",
             "key": "masthead-dark-logo.svg"
           }
         }
       },
       {
         "mode": "Light",
         "source":  {
           "from": "ConfigMap",
           "configMap": {
             "name": "custom-logos-config",
             "key": "masthead-light-logo.svg"
           }
       }
     ]
   },
   {
     "type": "Favicon",
     "themes": [
       {
         "mode": "Dark",
         "source": {
           "from": "ConfigMap",
           "configMap": {
             "name": "custom-logos-config",
             "key": "favicon-dark-logo.png"
           }
         }
       },
       {
         "mode": "Light",
         "source": {
           "from": "ConfigMap",
           "configMap": {
             "name": "custom-logos-config",
             "key": "favicon-light-logo.png"
           }
         }
       }
     ]
   }
 ]
}

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-bot · 2025-03-21T00:46:53Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-config-api
This PR has been included in build ose-cluster-config-api-container-v4.19.0-202503202308.p0.g75d64d7.assembly.stream.el9.
All builds following this will include this PR.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jan 28, 2025

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 28, 2025

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 28, 2025

openshift-ci bot requested review from bparees and knobunc January 28, 2025 22:24

cajieh force-pushed the ability-to-add-second-logo-pt6 branch 8 times, most recently from a7e82ea to 5947e08 Compare January 31, 2025 03:11

cajieh changed the title ~~[WIP] CONSOLE-4404: Add the ability to specify a second custom logo for PatternFly 6 [con…~~ [WIP] CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 [con… Jan 31, 2025

cajieh force-pushed the ability-to-add-second-logo-pt6 branch from 5947e08 to f503034 Compare January 31, 2025 12:39

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 31, 2025

cajieh changed the title ~~[WIP] CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 [con…~~ CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 [con… Jan 31, 2025

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2025

everettraven suggested changes Jan 31, 2025

View reviewed changes

cajieh force-pushed the ability-to-add-second-logo-pt6 branch 4 times, most recently from 08ce713 to 59f99fc Compare January 31, 2025 21:26

cajieh force-pushed the ability-to-add-second-logo-pt6 branch 3 times, most recently from 7464d04 to d6ca60e Compare March 20, 2025 11:23

cajieh force-pushed the ability-to-add-second-logo-pt6 branch 4 times, most recently from 7a131a1 to 84cc3c5 Compare March 20, 2025 11:51

ability-to-add-second-logo-pt6

4207302

cajieh force-pushed the ability-to-add-second-logo-pt6 branch from 84cc3c5 to 4207302 Compare March 20, 2025 13:20

cajieh commented Mar 20, 2025

View reviewed changes

openshift-ci bot changed the title ~~CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 [api]~~ OCPBUGS-53412 CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 Mar 20, 2025

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Mar 20, 2025

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Mar 20, 2025

openshift-ci bot requested a review from yapei March 20, 2025 16:46

JoelSpeed reviewed Mar 20, 2025

View reviewed changes

openshift-ci bot assigned JoelSpeed Mar 20, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2025

openshift-merge-bot bot merged commit 75d64d7 into openshift:master Mar 20, 2025
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-53412 CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 #2177

OCPBUGS-53412 CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 #2177

cajieh commented Jan 28, 2025 •

edited

Loading

openshift-ci-robot commented Jan 28, 2025 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Jan 28, 2025

openshift-ci-robot commented Jan 31, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 31, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 31, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 31, 2025 •

edited by openshift-ci bot

Loading

everettraven Jan 31, 2025

everettraven Jan 31, 2025

everettraven Jan 31, 2025

everettraven Jan 31, 2025

cajieh Jan 31, 2025

everettraven Jan 31, 2025

everettraven Jan 31, 2025

cajieh Feb 1, 2025 •

edited

Loading

everettraven Feb 3, 2025

JoelSpeed commented Mar 20, 2025

cajieh Mar 20, 2025

cajieh commented Mar 20, 2025

jhadvig commented Mar 20, 2025 •

edited

Loading

jhadvig commented Mar 20, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 20, 2025 •

edited by openshift-ci bot

Loading

Example of how the CustomLogos map to the ConfigMap:

jhadvig commented Mar 20, 2025

openshift-ci-robot commented Mar 20, 2025 •

edited by openshift-ci bot

Loading

JoelSpeed left a comment

openshift-ci bot commented Mar 20, 2025

openshift-ci-robot commented Mar 20, 2025

Example of how the CustomLogos map to the ConfigMap:

openshift-bot commented Mar 21, 2025

		// CustomFiles defines a configuration based on theme types for the console UI logo.
		type CustomFiles struct {

OCPBUGS-53412 CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 #2177

OCPBUGS-53412 CONSOLE-4448: Add the ability to specify a second custom logo for PatternFly 6 #2177

Conversation

cajieh commented Jan 28, 2025 • edited Loading

Example of how the CustomLogos map to the ConfigMap:

openshift-ci-robot commented Jan 28, 2025 • edited by openshift-ci bot Loading

openshift-ci bot commented Jan 28, 2025

openshift-ci-robot commented Jan 31, 2025 • edited by openshift-ci bot Loading

openshift-ci-robot commented Jan 31, 2025 • edited by openshift-ci bot Loading

openshift-ci-robot commented Jan 31, 2025 • edited by openshift-ci bot Loading

openshift-ci-robot commented Jan 31, 2025 • edited by openshift-ci bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cajieh Feb 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Mar 20, 2025

Choose a reason for hiding this comment

cajieh commented Mar 20, 2025

jhadvig commented Mar 20, 2025 • edited Loading

jhadvig commented Mar 20, 2025 • edited by openshift-ci bot Loading

openshift-ci-robot commented Mar 20, 2025 • edited by openshift-ci bot Loading

Example of how the CustomLogos map to the ConfigMap:

jhadvig commented Mar 20, 2025

openshift-ci-robot commented Mar 20, 2025 • edited by openshift-ci bot Loading

JoelSpeed left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 20, 2025

openshift-ci-robot commented Mar 20, 2025

Example of how the CustomLogos map to the ConfigMap:

openshift-bot commented Mar 21, 2025

cajieh commented Jan 28, 2025 •

edited

Loading

openshift-ci-robot commented Jan 28, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 31, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 31, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 31, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jan 31, 2025 •

edited by openshift-ci bot

Loading

cajieh Feb 1, 2025 •

edited

Loading

jhadvig commented Mar 20, 2025 •

edited

Loading

jhadvig commented Mar 20, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 20, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 20, 2025 •

edited by openshift-ci bot

Loading