upcoming: [UIE-9378] - DBaaS - Display connection pool section and table in Networking tab#13195
Conversation
| ._ctx.pools, | ||
| ...databaseQueries | ||
| .database('postgresql', databaseId) | ||
| ._ctx.connectionPools._ctx.paginated(params), |
There was a problem hiding this comment.
The connection pools query needed to accept pagination parameters, so I've updated this query and made a change in the keys.ts file. I also updated function name to include query for consistency with other similar functions in the file.
| flexDirection: 'column', | ||
| }, | ||
| }, | ||
| })); |
There was a problem hiding this comment.
These were common styles that I ended up reusing for the ActionsMenuWrapper used in the landing page table and for the new table for connection pools and for positioning settings items.
I've moved these shared styles to this new shared.styles.ts file to make it easier to reuse them. These styles can be seen in the DatabaseManageNetworking and the new DatabaseConnectionPools and DatabaseRow components. You'll see them pointing to this file now.
There may be other styles we can move here, so I plan to move them here as I go through other changes and come across them.
| <DatabaseManageNetworking database={database} /> | ||
| {flags.databasePgBouncer && database.engine === 'postgresql' && ( | ||
| <DatabaseConnectionPools database={database} /> | ||
| )} |
There was a problem hiding this comment.
Added these checks at the networking tab to conditionally render the new section based on the feature flag and database engine. It will only appear for postgresql database clusters.
|
|
||
| const flags = useFlags(); | ||
| const { classes } = useStyles(); | ||
| const { classes } = makeSettingsItemStyles(); |
There was a problem hiding this comment.
Moved these styles above to a shared file. See my other comment
| color: theme.tokens.alias.Content.Icon.Primary.Hover, | ||
| }, | ||
| })); | ||
| })(({ theme }) => getActionMenuWrapperStyles(theme)); |
There was a problem hiding this comment.
Moved these styles above to a shared file. See my other comment
| ), | ||
| setMethod('GET'), | ||
| setParams(params), | ||
| ); |
There was a problem hiding this comment.
Updated to accept params as we need to be able to pass pageSize and pageNumber to the request.
...manager/src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPools.tsx
Outdated
Show resolved
Hide resolved
...manager/src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPools.tsx
Outdated
Show resolved
Hide resolved
...manager/src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPools.tsx
Outdated
Show resolved
Hide resolved
| <div style={{ overflowX: 'auto', width: '100%' }}> | ||
| <Table | ||
| aria-label={'List of Connection pools'} | ||
| style={ |
There was a problem hiding this comment.
Can we move this style into a styled component since it's taking up a few lines?
There was a problem hiding this comment.
Do you mean a styled table component?
...manager/src/features/Databases/DatabaseDetail/DatabaseNetworking/DatabaseConnectionPools.tsx
Outdated
Show resolved
Hide resolved
| page={pagination.page} | ||
| pageSize={pagination.pageSize} | ||
| pageSizes={PAGE_SIZES} | ||
| style={{ |
There was a problem hiding this comment.
Same here, can we move styles to a styled component?
There was a problem hiding this comment.
As discussed, we'll look into addressing the additional styled components separately.
1a7819d to
fa11cc7
Compare
| .database('postgresql', databaseId) | ||
| ._ctx.connectionPools._ctx.pool(poolName), | ||
| enabled, | ||
| refetchInterval: 20000, |
There was a problem hiding this comment.
Is this refetchInterval intentional?
There was a problem hiding this comment.
ah right! I copied that logic from the calls we make for the table. I wasn't sure if we needed to refetch this. I can remove it for now and verify with their team if we need to refetch this data. I they do want to refetch it, I can verify how often we should and add it back later. Would that work?
There was a problem hiding this comment.
I've removed it for now as it looks like it's applied to the wrong query. This was meant for the table data query. I can ask the backend team about refetching to see if we need it and how often it should be.
bnussman-akamai
left a comment
There was a problem hiding this comment.
Looks good! Just had one question about loading/error states in this context
| if (connectionPoolsLoading) { | ||
| return <CircleProgress />; | ||
| } |
There was a problem hiding this comment.
Is this the desired loading and error state?
We could inline the loading state and error state into the table itself using https://design.linode.com/?path=/docs/components-table-tablerowloading--documentation
There was a problem hiding this comment.
@bnussman-akamai So the DatabaseConnectionPool component is currently using the cds table web component that was integrated in a previous pull request (#12989) for the database landing page. I opted to use the same setup from the DatabaseLanding for this component for consistency.
After checking the code for the cds-web component table, that table doesn't have an inline error state/loading behavior like we see in the Linode Table component linked above. So we can't implement this behavior for the connection pool table. So for now, I'm planning to follow the same state pattern we see on the landing page.
There was a problem hiding this comment.
Ah right. My fault. I forgot DBaaS uses the CDS Web component table. If there isn't an equivalent I don't have any issues with the current approach
…ble in Networking tab
…r and applying paginator min, results and page sizes
a631c31 to
8fee513
Compare
Cloud Manager UI test results🎉 865 passing tests on test run #6 ↗︎
|



Description 📝
This pull request is an update to DBaaS to display the Connection Pools section and table in the Networking tab in database details.
Changes 🔄
List any change(s) relevant to the reviewer.
Scope 🚢
Upon production release, changes in this PR will be visible to:
Target release date 🗓️
TBD
Preview 📷
How to test 🧪
Prerequisites
(How to setup test environment)
databasePgBouncerfeature flag enabled. This will appear asDatabase PgBouncerin the dev tools.databasevariable and add the following lines:Verification steps
(How to verify changes)
Verifying Display Behavior for section and table
postgresqldatabases, the newManage PgBouncer Connection Poolssection is now displayedVerify Feature Flag Rendering Checks
databasePgBouncerfeature flagManage PgBouncer Connection Poolssection is no longer displayed in the Networking tabVerify PostgreSQL Engine Check
databasePgBouncerfeature flagmysqlinstead.Manage PgBouncer Connection Poolsis no longer displayed even though the feature flag is switched on.Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅