-
Notifications
You must be signed in to change notification settings - Fork 367
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-8098] - DBaaS 2.0 Landing Page GA #11039
feat: [UIE-8098] - DBaaS 2.0 Landing Page GA #11039
Conversation
8aca711
to
13e8624
Compare
Coverage Report: ✅ |
13e8624
to
6f653bc
Compare
Looks like we have a failing unit test 👀 |
databaseLabel: string; | ||
inlineLabel?: string; | ||
handlers?: ActionHandlers; |
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.
Could we make handlers required in DatabaseRow
and DatabaseActionMenu
so we don't have to handle the undefined case?
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.
Done
allowList={allowList} | ||
onClose={onCloseAccesControls} | ||
open={isManageAccessControlsDialogOpen} | ||
updateDatabase={updateDatabase} |
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.
Can we move the useDatabaseMutation
/ updateDatabase
function into AddAccessControlDrawer
so that it doesn't need to be a prop?
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.
Done
setIsManageAccessControlsDialogOpen, | ||
] = React.useState(false); | ||
|
||
const [allowList, setAllowList] = React.useState<ExtendedIP[]>([]); |
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 shouldn't need to have allowList
as state in this component. Can we just pass selectedDatabase
to AddAccessControlDrawer
and handle all of the state inside of AddAccessControlDrawer
?
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.
Done
/> | ||
</> | ||
) : ( | ||
'' |
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.
Do we need this ''
? Can we just use the following?
{isNewDatabase && (
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.
Done
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.
Failed unit test in DatabaseLanding.test
on this branch. left a few comments, other than that functionality LGTM! Agreed with Banks feedback.
packages/manager/.changeset/pr-11039-upcoming-features-1727885138241.md
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Databases/DatabaseLanding/DatabaseActionMenu.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Databases/DatabaseLanding/DatabaseActionMenu.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Databases/DatabaseLanding/DatabaseLanding.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Databases/DatabaseLanding/DatabaseLogo.tsx
Outdated
Show resolved
Hide resolved
d15f7b8
to
d8e1227
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.
@corya-akamai Thank you for addressing the feedback. I left a minor comment to remove a redundant undefined check, and there is a unit test failure that should be fixed. Other than that, the PR is good to go.
packages/manager/src/features/Databases/DatabaseLanding/DatabaseLanding.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Databases/DatabaseLanding/DatabaseLanding.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Databases/DatabaseDetail/AddAccessControlDrawer.tsx
Outdated
Show resolved
Hide resolved
85ca6e2
to
2c689e4
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.
@corya-akamai Thank you for addressing all the feedback. Changes LGTM!
There are some failing unit tests that need to be addressed |
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.
2c689e4
to
0ed20a3
Compare
@bnussman-akamai Can you try now? |
@corya-akamai There still a unit test failing in this one |
0ed20a3
to
d950776
Compare
databaseId={id} | ||
databaseLabel={label} | ||
handlers={handlers!} | ||
></DatabaseActionMenu> |
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.
></DatabaseActionMenu> | |
/> |
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.
Done
{selectedDatabase && ( | ||
<> | ||
<DatabaseSettingsDeleteClusterDialog | ||
databaseEngine={selectedDatabase.engine} | ||
databaseID={selectedDatabase.id} | ||
databaseLabel={selectedDatabase?.label} | ||
onClose={() => setIsDeleteDialogOpen(false)} | ||
open={isDeleteDialogOpen} | ||
/> | ||
<DatabaseSettingsResetPasswordDialog | ||
databaseEngine={selectedDatabase.engine} | ||
databaseID={selectedDatabase.id} | ||
onClose={() => setIsResetPasswordsDialogOpen(false)} | ||
open={isResetPasswordsDialogOpen} | ||
/> | ||
<AddAccessControlDrawer | ||
database={selectedDatabase} | ||
onClose={onCloseAccesControls} | ||
open={isManageAccessControlsDialogOpen} | ||
/> | ||
</> | ||
)} | ||
</> | ||
)} |
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.
Because we are conditionally rendering these dialogs/drawers. The open/close animation is broken. It is noticeable on AddAccessControlDrawer
. We should consider keeping them rendered at all times. This is usually how we keep the animation working.
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.
Good catch @bnussman-akamai I'll address it. There also seems to be an issue with sorting that needs to be addressed before merging.
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.
@bnussman-akamai I updated so the animation is working. The sorting is not workin for the "new" tab but that is a backend issue they are working on..
a1e82b8
to
0ec2684
Compare
0ec2684
to
a42fa99
Compare
Test failures in CI were unrelated (Cypress failures on delete placement group spec and vpc landing page) and component test failures due to memory issues. Unit tests pass now. Going to merge. |
Cloud Manager E2E Run #6658
Run Properties:
|
Project |
Cloud Manager E2E
|
Run status |
Failed #6658
|
Run duration | 31m 27s |
Commit |
c8f33a73e3: feat: [UIE-8098] - DBaaS GA Landing Page (#11039)
|
Committer | mpolotsk-akamai |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
3
|
Flaky |
4
|
Pending |
2
|
Skipped |
0
|
Passing |
428
|
Tests for review
linodes/clone-linode.spec.ts • 1 failed test
Test | Artifacts | |
---|---|---|
clone linode > can clone a Linode from Linode details page |
Screenshots
Video
|
linodes/rebuild-linode.spec.ts • 1 failed test
Test | Artifacts | |
---|---|---|
rebuild linode > cannot rebuild a provisioning linode |
Screenshots
Video
|
placementGroups/delete-placement-groups.spec.ts • 1 failed test
Test | Artifacts | |
---|---|---|
Placement Group deletion > can delete with Linodes assigned when unexpected error show up and retry |
Screenshots
Video
|
placementGroups/delete-placement-groups.spec.ts • 1 flaky test
Test | Artifacts | |
---|---|---|
Placement Group deletion > can unassign Linode when unexpected error show up and reopen the dialog |
Screenshots
Video
|
linodes/backup-linode.spec.ts • 1 flaky test
Test | Artifacts | |
---|---|---|
linode backups > can capture a manual snapshot |
Screenshots
Video
|
domains/smoke-clone-domain.spec.ts • 1 flaky test
Test | Artifacts | |
---|---|---|
Clone a Domain > clones a domain |
Screenshots
Video
|
volumes/create-volume.smoke.spec.ts • 1 flaky test
Test | Artifacts | |
---|---|---|
volumes > does not allow creation of a volume with invalid pricing from volumes landing |
Screenshots
Video
|
Description 📝
DBaaS 2.0 Landing Page GA
Changes 🔄
List any change relevant to the reviewer.
Note: The suspend action menu item will be added with the suspend feature in a separate PR.
Target release date 🗓️
10/14/24
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
Note: For testing this change, ensure that DBaaS v2 is enabled, Beta is turned off, and the user has the "Managed Databases" account capability.
Verification steps
(How to verify changes)
As an Author I have considered 🤔
Check all that apply