Bug 1798858: Fix bug where installed operators list status is missin… #4270

rhamilto · 2020-02-10T14:38:26Z

…g or incorrect

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1798858

Before:
Status shows status.reason in lieu of status.phase, thus resulting in the bug.

After:
Status now shows status.phase, which aligns it with ClusterServiceVersion Overview status and fixes bug where SubscriptionTableRow Status appeared in the Deployment column. As a result of this change, there is no longer an indicator the operator has been copied (i.e., there is no longer a status of Copied as that was coming from status.reason). IMO, we should explore an alternate and more obvious way to indicate an operator has been copied without repurposing Status.

openshift-ci-robot · 2020-02-10T14:38:34Z

@rhamilto: This pull request references Bugzilla bug 1798858, 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.

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.

rhamilto · 2020-02-10T14:38:45Z

@tlwu2013 or @alecmerdler, can you provide insight as to why the table status was showing status.reason and not status.phase?

rhamilto · 2020-02-10T15:13:05Z

/retest

openshift-ci-robot · 2020-02-10T19:15:53Z

@rhamilto: This pull request references Bugzilla bug 1798858, which is valid.

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 · 2020-02-10T19:20:34Z

@rhamilto: This pull request references Bugzilla bug 1798858, which is valid.

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.

rhamilto · 2020-02-10T21:15:51Z

/retest

rhamilto · 2020-02-11T15:45:48Z

/retest

rhamilto · 2020-02-11T16:01:11Z

/retest

openshift-ci-robot · 2020-02-12T18:56:36Z

@rhamilto: This pull request references Bugzilla bug 1798858, which is valid.

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.

… or incorrect Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1798858

rhamilto · 2020-02-12T21:18:12Z

frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx

@@ -184,11 +172,15 @@ const ClusterServiceVersionStatus: React.FC<ClusterServiceVersionStatusProps> =
  obj,
  subscription,
 }) => {
-  const statusString = _.get(obj, 'status.reason', ClusterServiceVersionPhase.CSVPhaseUnknown);
-  const showSuccessIcon = statusString === 'Copied' || statusString === 'InstallSucceeded';
+  const status = _.get(obj, 'status.phase');


The CLI does not use a default value (if status.phase does not exist, nothing is shown).

rhamilto · 2020-02-12T21:20:39Z

frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx

@@ -363,14 +342,14 @@ export const FailedSubscriptionTableRow: React.FC<FailedSubscriptionTableRowProp
        <ResourceLink kind="Namespace" title={namespace} name={namespace} />
      </TableData>

+      {/* Status */}


This was overlooked in #3712.

openshift-ci-robot · 2020-02-12T21:49:10Z

@rhamilto: This pull request references Bugzilla bug 1798858, which is valid.

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 · 2020-02-13T18:40:36Z

@rhamilto: This pull request references Bugzilla bug 1798858, which is valid.

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 · 2020-02-13T18:41:52Z

@rhamilto: This pull request references Bugzilla bug 1798858, which is valid.

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.

rhamilto · 2020-02-13T19:30:05Z

@benjaminapetersen, @TheRealJon this is ready for review.

tlwu2013 · 2020-02-13T21:55:29Z

Hi @rhamilto , I'll share some background/history here.

Like you pointed out, "Installed Operators list view" uses status.reason for showing if the CSV is:
a). created in the same namespace as where the Operator (deployment, subscription, etc) being installed (i.e. InstallSucceeded), or
b). a Copied one (by OLM) to a particular namespace for exposing provided APIs since regular users don't have the access to the namespace where the Operator being installed.

This only happens when an Operator is installed as a "global Operator" (made available to all namespaces) and would be installed in "openshift-operators" namespace. In this case, only the CSV in "openshift-operators" namespace is the one being installed by the OLM (as InstallSucceeded), and the CSVs in the rest of the namespaces are those being Copied.

In the past, UI needs this clear differentiation because:
a). users can only "Uninstall Operator" (through "Remove Operator Subscription") when picking up the namespace where the Operator Subscription object exists (i.e. "openshift-operators" namespace).
b). folks prefer to have a UI exposes info/status that is more aligned to how the underlying API works.

Given that:
a). now from the UI, users can "Uninstall Operator" ("Remove or Edit Subscription") even from a "Copied CSV" in any namespaces (not necessarily have to be the same namespace as where the Subscription object lives).
b). the new OLM API will be gradually moving away from this "Copied CSV" behavior.

I feel it should be okay if we free up this CSV status distinction and replace status.reason with status.phase on the list view as well.

Wondering what other people's thoughts about this?
@alecmerdler , @dmesser , @alimobrem , @robszumski , @itsptk

rhamilto · 2020-02-14T14:13:47Z

Thanks, @tlwu2013! The background is very helpful.

itsptk · 2020-02-14T14:44:40Z

This makes sense to me.

I just want to include a link to #3755 (comment) in which the operator status was added to the status cards in dashboards, and at the time we agreed that status.phase should be used there (to match the details view they would see if they clicked an operator) and that we should revisit the list view in the future, which is what this bug is accomplishing.

FYI @andybraren

TheRealJon

/lgtm

openshift-ci-robot · 2020-02-14T16:02:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhamilto, TheRealJon

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:

~~frontend/OWNERS~~ [rhamilto]

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 · 2020-02-14T18:39:42Z

@rhamilto: All pull requests linked via external trackers have merged. Bugzilla bug 1798858 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.

rhamilto · 2020-02-14T18:42:09Z

/cherry-pick release-4.3

openshift-cherrypick-robot · 2020-02-14T18:42:21Z

@rhamilto: #4270 failed to apply on top of branch "release-4.3":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	frontend/packages/console-shared/src/components/status/Status.tsx
M	frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx
Falling back to patching base and 3-way merge...
Auto-merging frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx
CONFLICT (content): Merge conflict in frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx
Auto-merging frontend/packages/console-shared/src/components/status/Status.tsx
CONFLICT (content): Merge conflict in frontend/packages/console-shared/src/components/status/Status.tsx
Patch failed at 0001 Bug 1798858: Fix bug where installed operators list status is missing or incorrect

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.

rhamilto · 2020-02-14T19:32:09Z

frontend/packages/console-shared/src/components/status/Status.tsx

      return <ErrorStatus {...statusProps}>{children}</ErrorStatus>;

    case 'Accepted':
    case 'Active':
    case 'Bound':
    case 'Complete':
    case 'Completed':
+    case 'Copied':


This is not needed as this is a status.reason status.

rhamilto added the kind/bug Categorizes issue or PR as related to a bug. label Feb 10, 2020

rhamilto added this to the v4.4 milestone Feb 10, 2020

rhamilto assigned benjaminapetersen and spadgett Feb 10, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 10, 2020

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 10, 2020

openshift-ci-robot added component/olm Related to OLM component/shared Related to console-shared labels Feb 10, 2020

openshift-ci-robot requested review from jeff-phillips-18 and spadgett February 10, 2020 14:44

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2020

rhamilto force-pushed the bz1798858 branch from 2ecb061 to 5bcc87c Compare February 10, 2020 19:17

rhamilto force-pushed the bz1798858 branch from 5bcc87c to 67d9873 Compare February 12, 2020 18:54

Bug 1798858: Fix bug where installed operators list status is missing…

95037eb

… or incorrect Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1798858

rhamilto force-pushed the bz1798858 branch from 67d9873 to 95037eb Compare February 12, 2020 21:05

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 12, 2020

rhamilto commented Feb 12, 2020

View reviewed changes

rhamilto assigned TheRealJon Feb 13, 2020

rhamilto changed the title ~~[WIP] Bug 1798858: Fix bug where installed operators list status is missin…~~ Bug 1798858: Fix bug where installed operators list status is missin… Feb 13, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2020

rhamilto mentioned this pull request Feb 13, 2020

Bug 1797931: Make namespace, deployment cols sortable in Installed Operators table #4312

Merged

TheRealJon approved these changes Feb 14, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2020

openshift-merge-robot merged commit 977ce60 into openshift:master Feb 14, 2020

rhamilto deleted the bz1798858 branch February 14, 2020 18:40

rhamilto commented Feb 14, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1798858: Fix bug where installed operators list status is missin… #4270

Bug 1798858: Fix bug where installed operators list status is missin… #4270

rhamilto commented Feb 10, 2020 •

edited

openshift-ci-robot commented Feb 10, 2020

rhamilto commented Feb 10, 2020

rhamilto commented Feb 10, 2020

openshift-ci-robot commented Feb 10, 2020

openshift-ci-robot commented Feb 10, 2020

rhamilto commented Feb 10, 2020

rhamilto commented Feb 11, 2020

rhamilto commented Feb 11, 2020

openshift-ci-robot commented Feb 12, 2020

rhamilto Feb 12, 2020 •

edited

rhamilto Feb 12, 2020

openshift-ci-robot commented Feb 12, 2020

openshift-ci-robot commented Feb 13, 2020

openshift-ci-robot commented Feb 13, 2020

rhamilto commented Feb 13, 2020

tlwu2013 commented Feb 13, 2020 •

edited

rhamilto commented Feb 14, 2020

itsptk commented Feb 14, 2020

TheRealJon left a comment

openshift-ci-robot commented Feb 14, 2020

openshift-ci-robot commented Feb 14, 2020

rhamilto commented Feb 14, 2020

openshift-cherrypick-robot commented Feb 14, 2020

rhamilto Feb 14, 2020

Bug 1798858: Fix bug where installed operators list status is missin… #4270

Bug 1798858: Fix bug where installed operators list status is missin… #4270

Conversation

rhamilto commented Feb 10, 2020 • edited

openshift-ci-robot commented Feb 10, 2020

rhamilto commented Feb 10, 2020

rhamilto commented Feb 10, 2020

openshift-ci-robot commented Feb 10, 2020

openshift-ci-robot commented Feb 10, 2020

rhamilto commented Feb 10, 2020

rhamilto commented Feb 11, 2020

rhamilto commented Feb 11, 2020

openshift-ci-robot commented Feb 12, 2020

rhamilto Feb 12, 2020 • edited

Choose a reason for hiding this comment

rhamilto Feb 12, 2020

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 12, 2020

openshift-ci-robot commented Feb 13, 2020

openshift-ci-robot commented Feb 13, 2020

rhamilto commented Feb 13, 2020

tlwu2013 commented Feb 13, 2020 • edited

rhamilto commented Feb 14, 2020

itsptk commented Feb 14, 2020

TheRealJon left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 14, 2020

openshift-ci-robot commented Feb 14, 2020

rhamilto commented Feb 14, 2020

openshift-cherrypick-robot commented Feb 14, 2020

rhamilto Feb 14, 2020

Choose a reason for hiding this comment

rhamilto commented Feb 10, 2020 •

edited

rhamilto Feb 12, 2020 •

edited

tlwu2013 commented Feb 13, 2020 •

edited