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

CONSOLE-2805: Improve visibility of Operator installation status #9346

Merged

Conversation

rebeccaalpert
Copy link
Contributor

@rebeccaalpert rebeccaalpert commented Jun 25, 2021

Added install page logic to OperatorHub install states, disabled install button when operator is installing, and added a message.

Operator without approval:
Screen Shot 2021-06-28 at 10 06 08 AM

Operator without approval shows up in "Not Installed":
Screen Shot 2021-06-28 at 10 06 36 AM

Fixes https://issues.redhat.com/browse/CONSOLE-2805.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1970466

Design story: https://docs.google.com/document/d/1_YylQbRfFO0CI5Lmf9UDOHuIeOULHBY3vicI1x_RE38

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 25, 2021
@openshift-ci openshift-ci bot added component/olm Related to OLM kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Jun 25, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2021
@rebeccaalpert rebeccaalpert changed the title [WIP] Improve visibility of Operator installation status Improve visibility of Operator installation status Jun 28, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 28, 2021
@rebeccaalpert
Copy link
Contributor Author

CC @openshift/team-ux-review

Copy link
Member

@TheRealJon TheRealJon left a comment

Choose a reason for hiding this comment

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

Overall I think the logic works, but there were a few issues with type definitions and I think a bug where you are passing a list of Subscriptions to a function that expects a single instance. See comments for more detail.

A few other issues that I'm noticing with this:

  • We should probably just make OperatorHubItem["installed"] just a string type rather than adding a new boolean property and update the filters appropriately.
  • There are four states that an operator installation can be in: not installed, pending, installed, or failed. The failed state isn't accounted for in this implementation.
  • The install button in the operator details pane probably needs to change based on the install state. Previously it was just Install or Uninstall, but if the operator is in a pending or failed state, we need other states for that button as well.
  • Not sure if this was in the original design, but we might need to update the Installed badge that's shown on each item in the tile view. If it is installed but failed or pending, it would make sense to indicate that in the badge.

@spadgett spadgett changed the title Improve visibility of Operator installation status CONSOLE-2805: Improve visibility of Operator installation status Jun 28, 2021
@rebeccaalpert
Copy link
Contributor Author

Hi @TheRealJon - thanks for the review. I'll take a look at the code comments.

A few notes (we did handle these other states already):

  • We do handle the failure state in this implementation - please take another look at the "installed" and "isInstalling" logic.
  • We already do change the install button in the operator details pane based on the install state in this implementation.. Previously it was just Install or Uninstall. This "disabled" state is the state recommended by design.
  • We already do update the Installed badge in this implementation per UXD's suggestion.

@TheRealJon
Copy link
Member

@rebeccaalpert Thanks for clarifying those points, I did see that the button was disabled and that the installed badge is hidden. Maybe we hold off on removing the install property for now and just address the rest of the issues.

@TheRealJon
Copy link
Member

A few other issues that I'm noticing with this:

  • We should probably just make OperatorHubItem["installed"] just a string type rather than adding a new boolean property and update the filters appropriately.

Maybe hold off on this. I still think that the four install states would be better indicated by a string, but it's more complicated by the fact that we don't want the filter facets to have all four states listed.

  • There are four states that an operator installation can be in: not installed, pending, installed, or failed. The failed state isn't accounted for in this implementation.

My mistake, I went back and saw this is handled.

  • The install button in the operator details pane probably needs to change based on the install state. Previously it was just Install or Uninstall, but if the operator is in a pending or failed state, we need other states for that button as well.

I should have reviewed the design, which just called for a disabled button. I was looking for more than a one-line change on this.

  • Not sure if this was in the original design, but we might need to update the Installed badge that's shown on each item in the tile view. If it is installed but failed or pending, it would make sense to indicate that in the badge.

This was discussed in the design and decided against.

@rebeccaalpert
Copy link
Contributor Author

@TheRealJon - I addressed the PR feedback (with the exception of one prop destructuring comment, since we're passing some of the hidden props down to the child intentionally, but not using them in the parent otherwise). Can you please take another look when you get a chance? Thanks!

@rebeccaalpert rebeccaalpert force-pushed the operator-install-status branch 2 times, most recently from 858d95c to c26fc75 Compare July 2, 2021 19:40
Added install page logic to OperatorHub install states, disabled install button when operator is installing, and added a message.

Fixes https://issues.redhat.com/browse/CONSOLE-2805
Copy link
Member

@TheRealJon TheRealJon 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 openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 2, 2021
@rebeccaalpert
Copy link
Contributor Author

/assign @yapei @sferich888 @ahardin-rh
for approvals

@rebeccaalpert rebeccaalpert added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2021
@ahardin-rh
Copy link

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Jul 2, 2021
@itsptk
Copy link

itsptk commented Jul 6, 2021

@spadgett We talked about it some in the sprint demo, but the design for this story was generally created to be as simple as possible while still addressing the bug/RFE of showing the "Installed" badge when the operator hadn't actually installed.

We didn't think we would need to reflect the install status of operators in OperatorHub, since that is the purpose of the Installed Operators page, but we did still want to make sure we weren't misleading users that an operator had installed when it really had not.

I think the current design accomplishes this and probably doesn't need to reflect the status on the tile, but if we did want to add different messages to the blue Note that appears in the OperatorHub details sidepane for manual approval needed or errors, that would seem fine too. This would otherwise be accomplished by clicking the link to view the Subscription details which we thought might be sufficient for this "catalog" moment.

@TheRealJon
Copy link
Member

@spadgett We talked about it some in the sprint demo, but the design for this story was generally created to be as simple as possible while still addressing the bug/RFE of showing the "Installed" badge when the operator hadn't actually installed.

We didn't think we would need to reflect the install status of operators in OperatorHub, since that is the purpose of the Installed Operators page, but we did still want to make sure we weren't misleading users that an operator had installed when it really had not.

I think the current design accomplishes this and probably doesn't need to reflect the status on the tile, but if we did want to add different messages to the blue Note that appears in the OperatorHub details sidepane for manual approval needed or errors, that would seem fine too. This would otherwise be accomplished by clicking the link to view the Subscription details which we thought might be sufficient for this "catalog" moment.

My thought on this is that if we don't give the user a clear status on OperatorHub, they will assume an "in progress" state and just wait for it to resolve. I think in most cases (other than an update) if an Operator is stuck in an intermediate state for longer than a few seconds, there is probably something going on that requires user intervention. IMO there are actually 5 different possible install states that an operator can be in and those should be shown somewhere on the OperatorHub page: 'not installed', 'in progress', 'updating', 'requires approval', and 'failed'

Also, up until now, I don't think we've mentioned the 'updating' state. In the current implementation, an operator that's updating will be treated the same as one that is in progress, pending approval, or failed. Updating and in-progress states are arguably the same since the initial install is essentially just a special case for updating (updating from no version to requested version), but I think there is some distinction there.

@XiyunZhao
Copy link

This PR has been tested on private build by following the desgin document. Below listed part has been verified and did not find any issue

  1. "Installed" lable will be added for the operator which has been installed successfully, also it will be moved into "Installed" filter. For the other which is in installing, pending or failed state, it will be captured under the "Not Installed" facet.
  2. The install button is disabled on the details of the operator when the operator is on installing or being installed.
  3. The note "This Operator is being installed on the cluster. View it here" is displayed to the user, and user is able to use the hyerlink to jump into operator details Subscription tab

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 8, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rebeccaalpert, TheRealJon, XiyunZhao

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

@itsptk
Copy link

itsptk commented Jul 8, 2021

My thought on this is that if we don't give the user a clear status on OperatorHub, they will assume an "in progress" state and just wait for it to resolve.

I think what I generally struggle with is the idea that OperatorHub is a destination to find out what is happening with an operator install. It seems more likely to me that the user would head to the Installed Operators page and get a fairly full picture of an install process, if that's what they were interested in.

I could see a user caring about the install status in OperatorHub if they were either browsing for other operators and notice that an operator they previously thought they installed still doesn't say installed, or try and reinstall an operator they thought they installed but don't see it's functionality in the console (and in both cases they would see the Installing... note in the OperatorHub details view for that operator and be linked to its more detailed Subscription that conveys 'requires approval' or 'failed.') There was also the mentioned case where another user is browsing OperatorHub and should be able to see that an operator installation 'requires approval' or has 'failed,' though I wonder how likely this is that someone would happen across an operator tile in this state (and be the right person to help resolve them) and not just more intentionally discover these operator states from the Installed Operators page.

That said, I don't think it's harmful to convey these more detailed statuses on the tile and in the OperatorHub operator details, it just seemed like more implementation work and I didn't think it was absolutely necessary. The only way I think it would make for worse UX would be if there was some delay in updating the statuses on the tiles (tile still says Installing when the installation is now complete) or if there was other weirdness trying to aggregate statuses for multiple installs of the same operator in different namespaces (in the 'All projects' view, though I'm not even 100% certain how this works today just for the installed badge.)

FWIW I tried to look at OCM Add-Ons to see if there was any precedent to show 'Installing' on the tiles there but had trouble getting a cluster to test on or find designs conveying current behavior.

@TheRealJon
Copy link
Member

@itsptk Sorry, my original comment could have been clearer. I think it would be fine for the tile view badges and filter to work as originally designed. I was more focused on the alert in the details panel.

@rebeccaalpert
Copy link
Contributor Author

Hey @XiyunZhao - can you add the qe-approved label if this is all set? Thanks!

@sferich888
Copy link

/label px-approved
/unassign @sferich888

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Jul 19, 2021
@XiyunZhao
Copy link

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jul 20, 2021
@spadgett
Copy link
Member

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2021
@openshift-merge-robot openshift-merge-robot merged commit afec064 into openshift:master Jul 20, 2021
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
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/olm Related to OLM docs-approved Signifies that Docs has signed off on this PR kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants