-
Notifications
You must be signed in to change notification settings - Fork 631
CONSOLE-4404,CONSOLE-4062: Add the ability to specify second logo, favicons #14749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
CONSOLE-4404,CONSOLE-4062: Add the ability to specify second logo, favicons #14749
Conversation
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
cmd/bridge/main.go
Outdated
@@ -110,6 +110,8 @@ func main() { | |||
|
|||
fBranding := fs.String("branding", "okd", "Console branding for the masthead logo and title. One of okd, openshift, ocp, online, dedicated, azure, or rosa. Defaults to okd.") | |||
fCustomProductName := fs.String("custom-product-name", "", "Custom product name for console branding.") | |||
|
|||
fCustomLogoFiles := fs.String("custom-logo-files", "", "List of custom product images for console branding. Each entry consist of theme type as a key and path as a value.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update the info string
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
pkg/serverconfig/custom_logos.go
Outdated
serverutils.SendResponse(w, http.StatusBadRequest, serverutils.ApiError{Err: errMsg}) | ||
return | ||
} | ||
// logo is a public content and revalidate the each new request before releasing cached files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// logo is a public content and revalidate the each new request before releasing cached files | |
// logo is a public content, revalidate each new request before releasing cached files |
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
@Mylanos: 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:
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. |
d88af9e
to
b13008b
Compare
@Mylanos: 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:
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. |
b13008b
to
d65b514
Compare
Well that didn't work. 🤔 |
Right as I posted that it refreshed. 🥇 |
Found one bug in backend and one go.mod issue |
3e78d08
to
2a5c1d9
Compare
cmd/bridge/main.go
Outdated
fCustomLogoFile := fs.String("custom-logo-file", "", "Custom product image for console branding.") | ||
|
||
customLogoFlags := serverconfig.LogosKeyValue{} | ||
fs.Var(&customLogoFlags, "custom-logo-files", "List of custom product images used for console branding. Each entry consist of theme type (Dark | Light | Default) as a key and path to image file as value.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec.customization.customLogoFile
field is still valid for consoles.operator configuration although only one of spec.customization.customLogoFile
and spec.customization.logos
can take effect, shall we keep flag -custom-logo-file
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, even though spec.customization.customLogoFile
is marked as deprecated console.operator had to keep the configuration for backwards compatibility, so we won't affect customers still having the customLogoFile
OpenShift API configured. In reality what CO does in case there is a customLogoFile
OpenShift configuration still present is that it just feeds it to the new Logos
API to configure the Masthead type custom logo for all themes ( light and dark currently). So in the end console.operator will always pass the customLogo configuration to bridge/console via the new Logos
API inside the console-config.
I think we have decided to remove this configuration from bridge, due to its nature of being only a secondary API to console.operator, meaning this configuration is meant for developers of the bridge/console only and no real customer should be dependent on this.
cc @jhadvig @TheRealJon to confirm or deny.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense, thank you @Mylanos
cmd/bridge/main.go
Outdated
customLogoFlags := serverconfig.LogosKeyValue{} | ||
fs.Var(&customLogoFlags, "custom-logo-files", "List of custom product images used for console branding. Each entry consist of theme type (Dark | Light | Default) as a key and path to image file as value.") | ||
customFaviconFlags := serverconfig.LogosKeyValue{} | ||
fs.Var(&customFaviconFlags, "custom-favicon-files", "List of custom product images used for console branding. Each entry consist of theme type (Dark | Light | Default) as a key and path to image file as value.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we update the help message for -custom-favicon-files
flag? It's exactly the same as -custom-logo-files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will update.
In my testing, I don't see favicon is updated when switching between favicon-remains-same.movand here is my configuration
|
Another issue: any time user refreshs console page, it will show default masthead logo and favicon for a while, then custom masthead logo and favicon shows up. It's better not show default logo. |
This is expected. The favicon should only change when the system preference is changed. Why? That's what results in the browser's tab color changing. Note there is a known limitation because of this behavioral difference: if the user preference is set to |
This should only happen if custom product name is not set, which should be less common since we instruct users to set both. |
…tion if there is a favicon or customLogo customization
…ic logos when custom logos have not been fetched yet. Used the newly introduced SERVER_FLAGS customFaviconsConfigured and customLogosConfigured in a logic that prevents attempts to fetch custom logo type that is not configured.
@Mylanos: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Do the SVGs have width and height attributes defined (e.g., https://github.com/openshift/console/blob/main/frontend/public/imgs/openshift-logo.svg?short_path=340083c#L1)? I'm guessing they don't. |
If you use the same logos on the PatternFly web site, do they show up? |
@Mylanos @rhamilto The issue was that I saw empty masthead log when I only set custom masthead logo for one theme(light or dark). Eg, when I set custom masthead logo for dark theme, I could see the new masthead logo in dark theme, at the meanwhile I will see empty masthead log in light theme.
AwesomeScreenshot-4_21_2025.3_54_20PM.webm |
@yanpzhan Do you mind if we move forward with merging this and openshift/console-operator#963 and open a bug to address the above issue? |
IMO, this is expected behavior. I do not think we should make assumptions about a user's intent. It'll make troubleshooting much easier. |
They didn't have height or width attributes. Went back and reproduced with the original |
There is not blocking issue for current feature, we can discuss any confusion later, so approve the pr now. |
@Mylanos: 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. This pull request references CONSOLE-4062 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:
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. |
/retest |
After:
Screen.Recording.2025-02-10.at.18.06.06.mov
bridge command used for testing: