Skip to content
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

Improve Installed Operators list page #4830

Merged

Conversation

rebeccaalpert
Copy link
Contributor

@rebeccaalpert rebeccaalpert commented Mar 26, 2020

Fixes 98% of https://issues.redhat.com/browse/CONSOLE-2130.

Changes

  • I removed the "Deployment" column and added a "Last Updated" column.
  • If "All Projects" is selected in the project selector, there is 1 row per installation of an operator/subscription:
    • If Operator X is installed globally, there is 1 row
    • If Operator X is installed in one namespace, there is 1 row
    • If Operator X is installed twice in Namespace A and Namespace B, there are 2 rows, a row per install.
  • I added an "Operator Namespace" column indicating where the operator itself is installed (present on all projects view).
    • The column includes a "namespace-name" which is linked to the namespace
    • If the operator is installed globally, the muted text below "namespace-name" is "All Namespaces"
  • I added a "Managed Namespace(s)" column indicating the namespace(s) that the operator is watching.
    • If the operator is global, it lists "All Namepaces" in muted text
    • If the operator is not global, it lists the namespace(s) that it is watching
    • I added a popover to list out namespaces if the list is 2+ to ensure the row doesn't blow up
  • I changed the "Namespace" field on the Operator Details page to "Managed Namespace"
  • I changed the "Namespace" field on the Operator Subscriptions page to "Operator Namespace"

The PatternFly React table does not currently support popovers in sortable headers, so that will be done as a follow-on. (I created this issue to track this: https://issues.redhat.com/browse/CONSOLE-2168.)

Screenshots

Before:
Screen Shot 2020-03-30 at 12 12 15 PM

After (all projects):
Screen Shot 2020-03-30 at 5 31 04 PM

After (single project):
Screen Shot 2020-03-30 at 5 30 04 PM

Multinamespace:
Screen Shot 2020-03-30 at 12 25 53 PM

All namespaces:
Screen Shot 2020-03-30 at 2 41 04 PM

Subscription:
77947237-a0b0ff00-7291-11ea-96f3-b124ad29f7bb

CSV Details Page:
Screen Shot 2020-03-30 at 5 28 27 PM

Subscription Details Page:
Screen Shot 2020-03-31 at 12 22 07 PM

Heads up @openshift/team-ux-review.

@openshift-ci-robot 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 Mar 26, 2020
@rebeccaalpert rebeccaalpert force-pushed the installedoperators branch 3 times, most recently from 92ae2a2 to 5e52578 Compare March 26, 2020 17:48
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. component/core Related to console core functionality labels Mar 26, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2020
@rebeccaalpert rebeccaalpert force-pushed the installedoperators branch 3 times, most recently from e14c823 to babf9a1 Compare March 30, 2020 17:58
@rebeccaalpert rebeccaalpert changed the title [WIP] Installed operators page updates [WIP] Improve Installed Operators List Page Mar 31, 2020
@rebeccaalpert rebeccaalpert force-pushed the installedoperators branch 2 times, most recently from 9e63720 to a78f2b7 Compare March 31, 2020 16:46
@rebeccaalpert rebeccaalpert changed the title [WIP] Improve Installed Operators List Page Improve Installed Operators list page Mar 31, 2020
@openshift-ci-robot 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 Mar 31, 2020
@rachael-phillips
Copy link

Hi @rebeccaalpert still taking a look at this, but at first glance we should add The Namespace where the Operator is installed to the Subscription page popover.

image

@rebeccaalpert
Copy link
Contributor Author

All set @rachael-phillips!

Screen Shot 2020-03-31 at 2 03 08 PM

frontend/public/components/utils/details-page.tsx Outdated Show resolved Hide resolved
frontend/public/components/utils/details-item.tsx Outdated Show resolved Hide resolved
frontend/public/components/factory/table.tsx Outdated Show resolved Hide resolved
frontend/public/components/factory/table.tsx Outdated Show resolved Hide resolved
frontend/public/components/factory/table.tsx Outdated Show resolved Hide resolved
}

componentDidMount() {
const { columns } = this.state;
const componentProps: any = _.pick(this.props, [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid any type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't write this one and I'm honestly not sure how to better define the type given that it changes so much in action.

frontend/public/components/factory/table.tsx Outdated Show resolved Hide resolved
@spadgett
Copy link
Member

spadgett commented Apr 1, 2020

Hi @rebeccaalpert still taking a look at this, but at first glance we should add The Namespace where the Operator is installed to the Subscription page popover.

image

I don't think we should modify the popover.

These descriptions come entirely from the API documentation today. I think it's a bad idea to diverge from our API doc and special case certain fields in console with special documentation. What you see in the popover should match oc explain. The reason for this approach is to let the owners of the resources provide accurate documentation and to make sure it's always up-to-date (since API doc will be updated when APIs change).

Comment on lines 68 to 74
let label = 'Namespace';
if (kind === 'Subscription') {
label = 'Operator Namespace';
}
if (kind === 'ClusterServiceVersion') {
label = 'Managed Namespace';
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @christianvogt that this component should be generic and not know about these kinds.

From a UX standpoint, I also think this change is confusing as well. Calling it something other than Namespace makes me think the resource is in a different namespace and is just managing this namespace, which is not the case. It makes me think I can change the managed namespace, which I can't.

This component is here to ensure consistency across pages, and every other page calls this Namespace. Namespace also matches what the CLI and YAML show. It's hard to change this label because it should be hard to change this label. Changing it creates an inconsistency.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alimobrem @tlwu2013 @dmesser what are your thoughts on this? Curious to hear what you think, as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the value in indicating to the user what the actual labels are so that they can map them to the CLI/YAML, and also pulling the contents of the popover from API descriptions to keep them current. I do think though, that doesn't leave much room in these details screens to inject any additional UX niceness (further describing a namespace as "operator" or "managed" for example.)

Perhaps we could have some common paradigm where these labels are allowed to be changed in the UI (some sort of 'display name') and if that is present, add the actual resource label name is added to the popover contents for reference.

"Appears as Namespace in the ClusterServiceVersion YAML."

Alternatively, maybe these popovers could be divided into two sections, an optional 'ui-specified' description that appears first, followed by the API description.

It would seem unfortunate to try to further describe these two kinds of namespaces in the list view's column headers and not have any indication of which are which in the resource details.

@itsptk
Copy link

itsptk commented Apr 2, 2020

Subscription:
77947237-a0b0ff00-7291-11ea-96f3-b124ad29f7bb

@rebeccaalpert I was wondering what view this screen was conveying? Is this when there is no subscription associated with the CSV?

@rebeccaalpert
Copy link
Contributor Author

Hey @itsptk - Subscriptions show up in the Installed Operators view while the operator is installing or if it fails to install. (The row disappears once the operator is successfully installed and is replaced with the operator's details). This screen illustrates how subscriptions handle the new columns like "Managed Namespace."

@rebeccaalpert rebeccaalpert force-pushed the installedoperators branch 2 times, most recently from ed99ca9 to cdfb045 Compare April 15, 2020 15:32
@benjaminapetersen
Copy link
Contributor

Just a few px off on the sorting arrows yet. Not sure if that is new.

Screen Shot 2020-04-15 at 11 35 46 AM

@benjaminapetersen
Copy link
Contributor

benjaminapetersen commented Apr 15, 2020

When I click the view one more... link, i get redirected to the All Instances tab, rather than the Details tab with the list of Provided APIs.
Screen Shot 2020-04-15 at 11 40 13 AM

Goes here:
Screen Shot 2020-04-15 at 11 40 53 AM

Rather than here:
Screen Shot 2020-04-15 at 11 41 55 AM

Since this is an existing bug, creating a bugzilla:

@rebeccaalpert
Copy link
Contributor Author

I pushed the PR feedback I can implement right now.

@rebeccaalpert
Copy link
Contributor Author

Addressed round of PR feedback (refactored into component, renamed column classes, etc.)

@benjaminapetersen
Copy link
Contributor

/approve

👍 to the approach, getting closer, just a couple items left for the lgtm.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2020
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
Removed deployment column and added columns for last updated, managed namespace,and operator namespace (present only on All Projects view). If operators are installed globally, we now only show one row. If operators are installed in more than one namespace, we now have a popover to list the namespaces.

Fixes https://issues.redhat.com/browse/CONSOLE-2130
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, rachael-phillips, rebeccaalpert, spadgett

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@spadgett
Copy link
Member

/test frontend

@spadgett
Copy link
Member

/hold
for #5100

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020
@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020
@openshift-merge-robot openshift-merge-robot merged commit 90f1d17 into openshift:master Apr 18, 2020
@spadgett spadgett added this to the v4.5 milestone Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/olm Related to OLM lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants