-
Notifications
You must be signed in to change notification settings - Fork 90
Adding icon to logging ConsoleLink #857
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
Adding icon to logging ConsoleLink #857
Conversation
internal/kibana/route.go
Outdated
| loggingIcon, err := url.ParseRequestURI("https://github.com/openshift/elasticsearch-operator/blob/master/files/eo_icon.svg?raw=true") | ||
| if err != nil { | ||
| log.Error(err, "Invalid icon URL") | ||
| } | ||
|
|
||
| cl := console.NewConsoleLink(KibanaConsoleLinkName, kibanaURL, "Logging", loggingIcon.String(), "Observability") |
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.
This creates a direct dependency of looking up the icon content from Github on production clusters. This won't work as expected as per not all clusters have access to Github or allow network traffic to it. I suggest to make use of go embed features to load the content into the operator runtime. (See https://pkg.go.dev/embed)
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.
Sure, I kind of expected this would be an issue.
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.
@periklis I updated the PR with another workaround which is using a data URI representation of the icon instead of a link.
However, if go embed is still preferred, we will have to put the icon file in the same path as the function calling it, which is internal/kibana/ instead of 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.
LGTM and in sufficient for now. We don't need go embed here. It doesn't buy us much.
|
/approve |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: btaani, periklis 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:
Approvers can indicate their approval by writing |
|
/uncc @alanconway |
|
@btaani: all tests passed! 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/test-infra repository. I understand the commands that are listed here. |
|
/lgtm |
|
/cherry-pick release-5.4 |
|
@periklis: new pull request created: #861 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 kubernetes/test-infra repository. |
Description
The Logging link in the Application Shortcut (Red Hat Applications) should show an icon. This PR adds the icon image to the kibana console link.
Links