Bug 1970466: Console's OperatorHub leads users to unrelated install plan, if subscription does not have its own #9256
Conversation
openshift-ci
bot
added
do-not-merge/work-in-progress
|
@kdoberst: This pull request references Bugzilla bug 1970466, which is invalid:
Comment 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kdoberst The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
component/olm
|
/bugzilla refresh |
openshift-ci
bot
added
bugzilla/valid-bug
|
@kdoberst: This pull request references Bugzilla bug 1970466, 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
Requesting review from QA contact: 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/team-ux-leads. Can you please take a look at this PR? There is a new message that is introduced as part of this PR and looking for feedback. /assign @openshift/team-ux-leads |
|
/assign @openshift/team-ux-leads |
kdoberst
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest |
|
|
||
| if (!subscription?.status?.installedCSV) { | ||
| return ( | ||
| <HintBlock className="co-catalog-page__hint" title="Operator Not Yet Installed"> |
There was a problem hiding this comment.
Hey, Kim. We might look at the messaging here. It's odd to see an "Uninstall" button on the same page with "Operator Not Yet Installed." Is the operator is still installing or installation failed? Is there something in the Subscription resource that will let us give a more specific status to the user?
cc @tlwu2013
There was a problem hiding this comment.
I didn't see anything in the subscription resource that would indicate that the CSV hasn't been found vs a failure, but I can look again.
I would love some suggestions on what the UI should respond with installedCSV is undefined or missing. I just went with what was suggested in the bug.
There was a problem hiding this comment.
Looking below, I see we try to fallback to the Subscription link when there's no CSV. I think that's the intended behavior, and I'd do that for this case as well.
There was a problem hiding this comment.
Here is where this bug gets kind of weird. On line 71, installedCSV is created by passing information.
const [installedCSV] = useK8sWatchResource<ClusterServiceVersionKind>({
kind: referenceForModel(ClusterServiceVersionModel),
name: subscription?.status?.installedCSV,
namespace: subscription?.metadata?.namespace,
isList: false,
namespaced: true,
});
In the case described in the bug, subscription.staus.insatlledCSV is actually undefined, but a item is returned from useK8sWatchResource. So the logic used below on line 88:
const to = installedCSV
? `${nsPath}/clusterserviceversions/${installedCSV?.metadata?.name ?? ''}`
: `${nsPath}/subscriptions/${subscription.metadata.name ?? ''}`;
Would always return true. What is actually happening is that the results from the useK8sWatchResource appears to return information on another installed operator where there was a valid CSV.
The only piece of data that I could find to indicate this situation where need information is known except a valid CSV is checking for subscription?.status?.installedCSV.
There was a problem hiding this comment.
Oh weird. I'm seeing this too. We probably need to beef that logic up a bit more.
There was a problem hiding this comment.
I think Sam is right and #9346 does cover this case (since it overrides the install hint block), but I'll double check.
There was a problem hiding this comment.
Thanks @rebeccaalpert for taking a look.
There was a problem hiding this comment.
Hi @kdoberst! I think I might have written that, good catch. We probably need to add a condition there to make sure that nothing gets passed into useK8sWatchResource if subscription.status.installedCSV is empty. Something like:
const [installedCSV] = useK8sWatchResource<ClusterServiceVersionKind>(
subscription?.status?.installedCSV ? {
kind: referenceForModel(ClusterServiceVersionModel),
name: subscription?.status?.installedCSV,
namespace: subscription?.metadata?.namespace,
isList: false,
namespaced: true,
} : null);There was a problem hiding this comment.
@kdoberst - I tried the subscription YAML update in https://bugzilla.redhat.com/show_bug.cgi?id=1970466 against my PR and it doesn't trigger the new "isInstalling" hint block from what I can tell. I also wasn't seeing the error though. Did you use a different YAML to test it?
|
I think this bug is superseded by story https://issues.redhat.com/browse/CONSOLE-2805, which will add an "Installing" status to OperatorHub. |
|
Agreed that this would probably be replaced by CONSOLE-2805, which has UX design: https://docs.google.com/document/d/1_YylQbRfFO0CI5Lmf9UDOHuIeOULHBY3vicI1x_RE38/edit cc @tlwu2013 |
|
Closing in favor of #9346 |
|
@kdoberst: This pull request references Bugzilla bug 1970466. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW 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. |
Related to Bug 1970466
When viewing an operator's detail information, a message is shown if the installed CSV(startingCSV) is not found instead of displaying that the operator has been installed.
As coded, if a yaml contains an unknown starting CSV, when calling useK8sWatchResource, the name key is set to undefined. This causes usek8sWatchResource to return a perviously installed operator to be returned causing incorrect information to be shown.