Skip to content

Conversation

@TheRealJon
Copy link
Member

@TheRealJon TheRealJon commented Feb 12, 2019

Fixes https://jira.coreos.com/browse/CONSOLE-1262, https://jira.coreos.com/browse/CONSOLE-1269, and a few other minor issues I found on the cluster settings page along the way.

  • Always allow the user to initiate a cluster update if updates are currently available, regardless of the state of the current version.
  • Sort by status (descending) when linking to cluster operator tab from failing or in progress alerts. This will bring the operators that are failing or updating to the top of the list and group them together.
  • Add placeholder to update channel modal dropdown.
  • Move cluster version helper functions to a separate file.
  • Use consistent bold titles for all alerts
  • Change 'Updates Available' alert text if an update is currently progressing or failing.
  • Remove incorrect release notes link from cluster update modal.
  • Improve type definitions for ClusterVersion.

Updates Available
image

Update in progress:
image

Update failing:
image

@openshift/team-ux-review

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 12, 2019
Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

@TheRealJon when the current state is updating, are we only offering the Update to another version if there are multiple versions? or are we essentially allowing them to downgrade back to where they were coming from?

Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

and do we have a way to cancel the update, which would essentially roll back to the prior version? @beanh66 i don't remember if there is a requirement for this, but we may want to consider it from a design perspective

@TheRealJon
Copy link
Member Author

TheRealJon commented Feb 13, 2019

@serenamarie125

when the current state is updating, are we only offering the Update to another version if there are multiple versions? or are we essentially allowing them to downgrade back to where they were coming from?

This is completely dependent on the state of the underlying ClusterVersion resource. I think there is a short period of overlap where the current desired version might still show as an available update, but that resolves itself in a couple of seconds. Essentially, the updates available alert will always show when the status.availableUpdates property of the ClusterVersion resource is not empty.

and do we have a way to cancel the update, which would essentially roll back to the prior version?

According to @spadgett, there is no concept of cancelling or rolling back an update (yet?).

@TheRealJon
Copy link
Member Author

/retest

@serenamarie125
Copy link
Contributor

This is completely dependent on the state of the underlying ClusterVersion resource. I think there is a short period of overlap where the current desired version might still show as an available update, but that resolves itself in a couple of seconds. Essentially, the updates available alert will always show when the status.availableUpdates property of the ClusterVersion resource is not empty.

@TheRealJon ok great, wanted to make sure it would only show up when it made sense.

Regarding cancel, now i remember a discussion with @spadgett and @beanh66 about cancel not being available for 4.0

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

When do we need to resolve this TODO?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not for beta2. The link that we had here was just not useful, so I removed it until we have something real to put there.

Copy link

@beanh66 beanh66 left a comment

Choose a reason for hiding this comment

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

@TheRealJon We reviewed this in the design meeting yesterday and the same questions arose around whether or not a user can rollback to a previous version or cancel. If neither is available, I'm wondering when that failing message goes away currently? If I take the action to try another version, will that remove the old failing message?

Other question is around the "view detailed progress" link. Does that take users to the Cluster Operators tab? If so, can we change to be more consistent with the failed message and explicitly say "View Cluster Operators for details" with Cluster Operators being the link?

@TheRealJon
Copy link
Member Author

/retest

@TheRealJon
Copy link
Member Author

@beanh66

I'm wondering when that failing message goes away currently? If I take the action to try another version, will that remove the old failing message?

The failing message will stay there as long as the underlying ClusterVersion resource has a failure condition in status.conditions. Starting a new update should clear that alert eventually, but it won't be instantaneous. It takes a little bit of time for the state changes to propagate through the system and update the UI, so there may be some delay.

Other question is around the "view detailed progress" link. Does that take users to the Cluster Operators tab? If so, can we change to be more consistent with the failed message and explicitly say "View Cluster Operators for details" with Cluster Operators being the link?

Yes, and I'll update the text to be more consistent.

@TheRealJon
Copy link
Member Author

/retest

1 similar comment
@TheRealJon
Copy link
Member Author

/retest

@TheRealJon
Copy link
Member Author

/retest

@benjaminapetersen
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019
@benjaminapetersen
Copy link
Contributor

/retest

5 similar comments
@TheRealJon
Copy link
Member Author

/retest

@TheRealJon
Copy link
Member Author

/retest

@TheRealJon
Copy link
Member Author

/retest

@TheRealJon
Copy link
Member Author

/retest

@TheRealJon
Copy link
Member Author

/retest

@benjaminapetersen
Copy link
Contributor

CI may struggle until this merges:
openshift/cluster-authentication-operator#73

I see the same issues in the console pod logs.

@spadgett
Copy link
Member

@TheRealJon Can you confirm that the test failures aren't real? Login is failing on this PR, and it's not a common flake.

@spadgett
Copy link
Member

/retest

1 similar comment
@spadgett
Copy link
Member

/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2019
@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 27, 2019
@spadgett
Copy link
Member

Install failed

/retest

@spadgett
Copy link
Member

/retest

3 similar comments
@spadgett
Copy link
Member

/retest

@spadgett
Copy link
Member

/retest

@spadgett
Copy link
Member

spadgett commented Mar 1, 2019

/retest

@jhadvig
Copy link
Member

jhadvig commented Mar 1, 2019

1) Basic console test : creates test namespace test-yvqmf if necessary
   Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
       at ontimeout timers.js:427:11
       at tryOnTimeout timers.js:289:5
       at listOnTimeout timers.js:252:5
       at Timer.processTimers timers.js:212:10
   Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
       at ontimeout timers.js:427:11
       at tryOnTimeout timers.js:289:5
       at listOnTimeout timers.js:252:5
       at Timer.processTimers timers.js:212:10

/retest

@spadgett
Copy link
Member

spadgett commented Mar 1, 2019

I'm seeing that error a lot on this PR and not on others. We should make sure it's not something in the changes.

@spadgett
Copy link
Member

spadgett commented Mar 1, 2019

/hold

Even with our flakes, this should have passed CI by now. I'd like to investigate.

@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 Mar 1, 2019
@spadgett
Copy link
Member

/hold cancel

We'll see if this passes CI after rebase

@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 Mar 11, 2019
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2019
@TheRealJon
Copy link
Member Author

@spadgett rebased

@spadgett
Copy link
Member

level=error msg="\t* aws_internet_gateway.igw: error attaching EC2 Internet Gateway (igw-07cfa5d11f9698241): timeout while waiting for state to become 'success' (timeout: 2m0s)"

/retest

@spadgett
Copy link
Member

level=error msg="\t* aws_internet_gateway.igw: error attaching EC2 Internet Gateway (igw-07b7d10ab7fa4c625): timeout while waiting for state to become 'success' (timeout: 2m0s)"

/retest

@spadgett
Copy link
Member

/retest

@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2019
@openshift-merge-robot openshift-merge-robot merged commit abdf645 into openshift:master Mar 12, 2019
@TheRealJon TheRealJon deleted the CONSOLE-1262 branch March 12, 2019 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants