Bug 1828922: Fix to get the CSV icon for operator workloads #5218
Conversation
openshift-ci-robot
added
kind/bug
|
@jeff-phillips-18: This pull request references Bugzilla bug 1828922, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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. |
openshift-ci-robot
added
bugzilla/valid-bug
| const csvIcon = | ||
| _.get(operatorBackedServiceKindMap[nodeResourceKind], 'spec.icon.0') || | ||
| _.get(operator, 'spec.icon.0'); |
There was a problem hiding this comment.
| const csvIcon = | |
| _.get(operatorBackedServiceKindMap[nodeResourceKind], 'spec.icon.0') || | |
| _.get(operator, 'spec.icon.0'); | |
| const csvIcon = _.get(operator, 'spec.icon.0'); |
You can simply do this because if you see above there is a check for operator and if it is empty it is assigned to operatorBackedServiceKindMap[nodeResourceKind] so we can get the icon from operator itself in both the cases.
There was a problem hiding this comment.
Both could exist. We don't attempt to look at operatorBackedServiceKindMap[nodeResourceKind] if we found the operator via the ownerUid. This will consider both.
There was a problem hiding this comment.
So you mean to say if we get the operator via the ownerUid then also for the icon we will give precedence to operatorBackedServiceKindMap[nodeResourceKind] and only if we dont get the icon from there then we will try to get it from operator that we got via the ownerUid?
There was a problem hiding this comment.
I could reverse the order to change the precedence. I left it this way thinking the original author may have purposefully chosen to use the icon from the kind map.
There was a problem hiding this comment.
Got it. I think this order should be fine since we should show the icon for the particular operator backed service and if there is none then use the operator icon.
There was a problem hiding this comment.
I would prefer that we move away from using lodash get everywhere and use optional chaining instead.
|
/lgtm |
|
@jeff-phillips-18 did you confirm this change with @serenamarie125 and @sspeiche ? The jira doesn't have a conclusive answer since the operator icon is already shown in the group label. |
|
@christianvogt FYI, the design documentation explains that the icon of each of the workloads inside of the operator backed service should follow this order: runtime, operator backed service, OLM/lightning bolt. We should be good! Just requested some screenies of before/after though! |
|
We are looking for the runtime builder image first, then for the operator image (now in both places) then fallback to default operator icon. I believe this matches what has been said in the Jira ticket, there just are not runtime builder images specified for these workloads. For the runtime image, we look at both labels: |
jeff-phillips-18
force-pushed
the
operator-icons
branch
from
ff4ac5b
to
1bf21a5
Compare
|
Before After |
|
/approve cc @christianvogt is there anything following up you want discussed here? |
openshift-ci-robot
added
the
do-not-merge/hold
openshift-ci-robot
added
lgtm
|
Nope i think we're good now |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, divyanshiGupta, jeff-phillips-18, serenamarie125 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 |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
@jeff-phillips-18: All pull requests linked via external trackers have merged: openshift/console#5218. Bugzilla bug 1828922 has been moved to the MODIFIED state. 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. |
|
This PR was merged in the time period when ci-operator was mistakenly reporting failed tests as passing. If this repository has ci-operator jobs, please inspect their test results even if passing, and consider the need for fixing or even reverting. |
Fixes:
https://issues.redhat.com/browse/ODC-2886
Analysis / Root cause:
Icon does not always exist on the CSV for the kind.
Solution Description:
Check the CSV for both the kind and the specific owner reference uid
/kind bug