-
Notifications
You must be signed in to change notification settings - Fork 361
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
feat: [UIE-8009] - DBaaS enhancements Backups #10961
feat: [UIE-8009] - DBaaS enhancements Backups #10961
Conversation
ee0608f
to
64e161a
Compare
Coverage Report: โ
|
64e161a
to
3c8396c
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.
This PR contains some changes that would have been best separated out into their own PRs (the creation of new components - which we typically abstract, style, unit test, and add to Storybook). If DBaaS is willing to use more basic components for date and/or time selection for now, the Cloud Manager team would then follow up with the core components in an M3 ticket to do the abstraction with calendar and time components.
As far as the actual functionality of the Backups tab for new DBs - the changes worked and I did not see any regressions for legacy DBs Backups.
I did leave some feedback throughout.
packages/manager/src/features/Databases/DatabaseDetail/DatabaseBackups/DatabaseBackups.tsx
Outdated
Show resolved
Hide resolved
...manager/src/features/Databases/DatabaseDetail/DatabaseBackups/RestoreNewFromBackupDialog.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Databases/DatabaseDetail/DatabaseBackups/DatabaseBackups.tsx
Show resolved
Hide resolved
...manager/src/features/Databases/DatabaseDetail/DatabaseBackups/RestoreNewFromBackupDialog.tsx
Show resolved
Hide resolved
<Grid item lg={3} md={4} xs={12}> | ||
<Typography variant="h3">Time(UTC)</Typography> | ||
<LocalizationProvider dateAdapter={AdapterLuxon}> | ||
<StyledTimePicker |
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.
This component has some styling issues, which are especially visible in dark mode. The double scroll bars, different colored options in dark mode, extra spacing before the OK button, and the transparency of the button itself are things we'd want to address. Building out our own TimePicker component is how we'd style this more consistently, rather than a direct import from MUI.
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.
@gitkcosby Wanted to check in with you about this. We have planned to build a date picker but it isn't available yet. Here, @mpolotsk-akamai has imported the component straight from MUI which is problematic for a variety of reasons (functionality, styling, testing, reliability) and we'd like to ask for a grace period to build this right and make it available to the Cloud Manager ecosystem the right way.
I'd like to suggest using a <Texfield type="date"><Texfield type="date">
combo during this grace period and replace it once the has been built, which I estimate to be done one or two releases following the DBaaS 9/30 beta release (in short we'd live with the temp solution for a sprint or two).
Any thoughts?
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.
@abailly-akamai , I've updated the time picker, to match the one in Settings Tab, for consistency.
...manager/src/features/Databases/DatabaseDetail/DatabaseBackups/RestoreNewFromBackupDialog.tsx
Outdated
Show resolved
Hide resolved
...manager/src/features/Databases/DatabaseDetail/DatabaseBackups/RestoreNewFromBackupDialog.tsx
Outdated
Show resolved
Hide resolved
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.
Thanks for the contribution @mpolotsk-akamai - great PR and attention to testing and coding conventions. I added a few comments to clean things up and let's see what we do with the picker before moving forward
packages/manager/src/features/Databases/DatabaseDetail/DatabaseBackups/DatabaseBackups.tsx
Show resolved
Hide resolved
...manager/src/features/Databases/DatabaseDetail/DatabaseBackups/RestoreNewFromBackupDialog.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Databases/DatabaseDetail/DatabaseBackups/DatabaseBackups.tsx
Outdated
Show resolved
Hide resolved
@mjac0bs , @abailly-akamai thank you for the review. |
99e3a75
to
5854ae3
Compare
5854ae3
to
1d0575d
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.
I'm approving pending CI finishes and looks good.
Just so we're all on the same page (cc @abailly-akamai , @gitkcosby) - we're on board with keeping the MUI imported calendar at the moment, because this use case (specific day) does differ a little from the Settings tab, which is only selecting a week day?
@mpolotsk-akamai @mjac0bs @gitkcosby I believe we agreed on using a day select as well and removing this version of the calendar waiting for the real component. This is the request that was agreed upon. Pretty much except it would be a date field on the left |
@abailly-akamai , based on our discussion with @gitkcosby , @corya-akamai weโll proceed with the MUI Calendar and a standard drop down for selecting a time. |
Right. @mpolotsk-akamai, @mpolotsk-akamai, @corya-akamai - I think @abailly-akamai's recommendation of |
@mjac0bs I cleared it with @gitkcosby - we'll keep it for DBaaS and we'll do as quick a follow up as possible to replace this instance soon, hoping no more pop up. If we catch other instances in other PRs we can prevent spread. I am reviewing the e2e failures now but this should be good to go otherwise |
Note that the |
Description ๐
DBaaS 2.0 Backups
Changes ๐
Target release date ๐๏ธ
9/16/24
Preview ๐ท
How to test ๐งช
Note:
To test all beta features, beta must be enabled, and the user must have the "Managed Databases Beta" account capability.
Prerequisites
(How to setup test environment)
Enable DatabaseV2Beta flag
Verification steps
(How to verify changes)
As an Author I have considered ๐ค
Check all that apply