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
Hide upgrade paths and show checklist progress during upgrades #5972
Hide upgrade paths and show checklist progress during upgrades #5972
Conversation
0e51b0e
to
e19fb29
Compare
334a70c
to
281ac2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
frontend/public/components/cluster-settings/cluster-settings.tsx
Outdated
Show resolved
Hide resolved
frontend/public/components/cluster-settings/cluster-settings.tsx
Outdated
Show resolved
Hide resolved
{ | ||
kind: referenceForModel(MachineConfigPoolModel), | ||
isList: true, | ||
prop: 'machineConfigPools', | ||
}, | ||
{ | ||
kind: referenceForModel(ClusterOperatorModel), | ||
isList: true, | ||
prop: 'clusterOperators', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these watches into the UpdateProgress
component? Then we only watch when an upgrade is in progress. (Consider the useK8sWatch
hook instead of Firehose
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to keep clusterOperators
here as <UpdateStatus>
is now using it as well, but machineConfigPools
can certainly move to <UpdateProgress>
. Good catch! It prompted me to notice I was plumbing machineConfigPools
through <UpdateStatus>
and <UpdatingMessage>
but not actually using it (orphaned from previous revisions).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've addressed this. PTAL, @spadgett.
@@ -626,6 +850,16 @@ export const ClusterSettingsPage: React.SFC<ClusterSettingsPageProps> = ({ match | |||
const resources = [ | |||
{ kind: clusterVersionReference, name: 'version', isList: false, prop: 'obj' }, | |||
{ kind: clusterAutoscalerReference, isList: true, prop: 'autoscalers', optional: true }, | |||
{ | |||
kind: referenceForModel(MachineConfigPoolModel), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to handle cases where the user doesn't have authority to watch MCPs or the one of the expected MCPs is not there. (For instance, you can have a 3 node cluster without workers.)
Make this optional
and hide the progress bar if the MCP is missing or has 0 replicas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've addressed this. PTAL, @spadgett.
frontend/public/components/cluster-settings/cluster-settings.tsx
Outdated
Show resolved
Hide resolved
frontend/public/components/cluster-settings/cluster-settings.tsx
Outdated
Show resolved
Hide resolved
/retest |
f274611
to
7d082e3
Compare
7d082e3
to
d48284f
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto, 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 |
Watch an upgrade run: https://www.youtube.com/watch?v=TnVbzkFYlvA
View conditions
link in action: https://youtu.be/Zm8_Bnum9vIView conditions
link in action: https://youtu.be/9l9QFjKKwRk