mgr/dashboard: cluster upgrade start UI #52493

avanthakkar · 2023-07-17T18:15:32Z

Fixes: https://tracker.ceph.com/issues/61928
Signed-off-by: avanthakkar avanjohn@gmail.com

Screencast.from.2023-07-24.15-39-08.webm

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

cloudbehl · 2023-07-18T08:11:55Z

Some Nits:

Are we checking two times if the update is available? once when we load the upgrade page and once we are opening the modal for upgrade, can't we just pass data from parent component to child and use it?

Also set the default height and width of modal to size you think it will require.

avanthakkar · 2023-07-18T10:59:31Z

Some Nits:

Are we checking two times if the update is available? once when we load the upgrade page and once we are opening the modal for upgrade, can't we just pass data from parent component to child and use it?

Also set the default height and width of modal to size you think it will require.

We need to subscribe again to see which versions are available. So probably I can update the message as
Fetching the available versions? Not sure if I can @input these versions. wdyt? @nizamial09
Sure will do some resizing for text and modal.

nizamial09

I've made some modifications in the review. If you follow that, you can avoid the duplicated api call.

Also the upgrade-start component you created should really be upgrade-form component and it should be inside upgrade/upgrade-form folder rather than existing in the same folder as upgrade.

Also it is missing the unit-test file. You should also create that and push it.

src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/upgrade/upgrade-start-modal-component.ts

nizamial09 · 2023-07-19T07:38:58Z

src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/upgrade/upgrade-start-modal-component.ts

+    this.upgradeInfo$ = this.upgradeService.list();
+    this.upgradeInfoError$ = this.upgradeInfo$?.pipe(


we don't need to do this again. We can pass the versions from the upgrade.component where we are actually doing the subscription. so on this file you can introduce a variable called versions to catch the params that are send from upgrade component.

Thanks for the help here @nizamial09 ! I've addressed the suggestions.

src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/upgrade/upgrade-start-modal-component.ts

src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/upgrade/upgrade.component.ts

src/pybind/mgr/dashboard/frontend/src/app/shared/constants/app.constants.ts

...ybind/mgr/dashboard/frontend/src/app/ceph/cluster/upgrade/upgrade-start-modal-component.html

cloudbehl · 2023-07-20T11:30:07Z

The version IMO needs to be in ascending order.

avanthakkar · 2023-07-24T10:10:08Z

The version IMO needs to be in ascending order.

Done!

avanthakkar · 2023-07-24T12:51:55Z

jenkins test make check

cloudbehl · 2023-07-25T07:21:36Z

Nit: New versions -> New version

nizamial09 · 2023-07-25T10:57:36Z

jenkins test dashboard cephadm

Pegonzal

Hey @avanthakkar , overall looks good to me and is working so I'll approve it!

But I agree with Nizam's comment which hasn't been taking into account yet, I think this should be done:

Also the upgrade-start component you created should really be upgrade-form component and it should be inside upgrade/upgrade-form folder rather than existing in the same folder as upgrade.

Also it is missing the unit-test file. You should also create that and push it.

avanthakkar · 2023-07-27T09:58:56Z

Hey @avanthakkar , overall looks good to me and is working so I'll approve it!

But I agree with Nizam's comment which hasn't been taking into account yet, I think this should be done:

Also the upgrade-start component you created should really be upgrade-form component and it should be inside upgrade/upgrade-form folder rather than existing in the same folder as upgrade.
Also it is missing the unit-test file. You should also create that and push it.

yeah missed that, will do unit test and move start modal component under upgrade-form

Fixes: https://tracker.ceph.com/issues/61928 Signed-off-by: avanthakkar <avanjohn@gmail.com>

avanthakkar · 2023-07-31T09:33:21Z

Nit: New versions -> New version

Done!

avanthakkar · 2023-07-31T09:33:29Z

Hey @avanthakkar , overall looks good to me and is working so I'll approve it!

But I agree with Nizam's comment which hasn't been taking into account yet, I think this should be done:

Also the upgrade-start component you created should really be upgrade-form component and it should be inside upgrade/upgrade-form folder rather than existing in the same folder as upgrade.
Also it is missing the unit-test file. You should also create that and push it.

Done!

avanthakkar · 2023-08-01T07:05:35Z

jenkins test make check

avanthakkar requested a review from a team as a code owner July 17, 2023 18:15

avanthakkar requested review from pereman2 and nizamial09 and removed request for a team July 17, 2023 18:15

github-actions bot added dashboard pybind labels Jul 17, 2023

avanthakkar force-pushed the cluster-upgrade-ui branch from f88786c to 33f4efb Compare July 18, 2023 10:21

avanthakkar force-pushed the cluster-upgrade-ui branch from 33f4efb to 9443352 Compare July 19, 2023 07:03

nizamial09 requested changes Jul 19, 2023

View reviewed changes

avanthakkar force-pushed the cluster-upgrade-ui branch from 9443352 to df3e7f0 Compare July 19, 2023 12:07

nizamial09 mentioned this pull request Jul 19, 2023

mgr/dashboard: add logs and daemon version to upgrade UI #52395

Merged

14 tasks

avanthakkar requested a review from nizamial09 July 20, 2023 07:04

avanthakkar force-pushed the cluster-upgrade-ui branch from df3e7f0 to f32212d Compare July 24, 2023 10:11

nizamial09 approved these changes Jul 25, 2023

View reviewed changes

Pegonzal approved these changes Jul 27, 2023

View reviewed changes

mgr/dashboard: cluster upgrade start UI

31055fa

Fixes: https://tracker.ceph.com/issues/61928 Signed-off-by: avanthakkar <avanjohn@gmail.com>

avanthakkar force-pushed the cluster-upgrade-ui branch from f32212d to 31055fa Compare July 31, 2023 08:44

Pegonzal merged commit 172133f into ceph:main Aug 1, 2023
11 of 13 checks passed

Pegonzal deleted the cluster-upgrade-ui branch August 1, 2023 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mgr/dashboard: cluster upgrade start UI #52493

mgr/dashboard: cluster upgrade start UI #52493

avanthakkar commented Jul 17, 2023 •

edited

cloudbehl commented Jul 18, 2023

avanthakkar commented Jul 18, 2023

nizamial09 left a comment

nizamial09 Jul 19, 2023

avanthakkar Jul 19, 2023

cloudbehl commented Jul 20, 2023

avanthakkar commented Jul 24, 2023

avanthakkar commented Jul 24, 2023

cloudbehl commented Jul 25, 2023

nizamial09 commented Jul 25, 2023

Pegonzal left a comment

avanthakkar commented Jul 27, 2023

avanthakkar commented Jul 31, 2023

avanthakkar commented Jul 31, 2023

avanthakkar commented Aug 1, 2023

		this.upgradeInfo$ = this.upgradeService.list();
		this.upgradeInfoError$ = this.upgradeInfo$?.pipe(

mgr/dashboard: cluster upgrade start UI #52493

mgr/dashboard: cluster upgrade start UI #52493

Conversation

avanthakkar commented Jul 17, 2023 • edited

Contribution Guidelines

Checklist

cloudbehl commented Jul 18, 2023

avanthakkar commented Jul 18, 2023

nizamial09 left a comment

Choose a reason for hiding this comment

nizamial09 Jul 19, 2023

Choose a reason for hiding this comment

avanthakkar Jul 19, 2023

Choose a reason for hiding this comment

cloudbehl commented Jul 20, 2023

avanthakkar commented Jul 24, 2023

avanthakkar commented Jul 24, 2023

cloudbehl commented Jul 25, 2023

nizamial09 commented Jul 25, 2023

Pegonzal left a comment

Choose a reason for hiding this comment

avanthakkar commented Jul 27, 2023

avanthakkar commented Jul 31, 2023

avanthakkar commented Jul 31, 2023

avanthakkar commented Aug 1, 2023

avanthakkar commented Jul 17, 2023 •

edited