-
Notifications
You must be signed in to change notification settings - Fork 399
feat: [M3-7257] - Invalidate VPC-related queries after deleting a linode #9814
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
Changes from all commits
c303787
dba97b9
dfe0307
d511015
964dad7
649ee8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@linode/manager": Upcoming Features | ||
| --- | ||
|
|
||
| Invalidate VPC-related queries when deleting a Linode ([#9814](https://github.com/linode/manager/pull/9814)) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import * as React from 'react'; | ||
| import { useQueryClient } from 'react-query'; | ||
|
|
||
| import { Notice } from 'src/components/Notice/Notice'; | ||
| import { TypeToConfirmDialog } from 'src/components/TypeToConfirmDialog/TypeToConfirmDialog'; | ||
|
|
@@ -9,6 +10,14 @@ import { | |
| useLinodeQuery, | ||
| } from 'src/queries/linodes/linodes'; | ||
|
|
||
| import { useAllLinodeConfigsQuery } from 'src/queries/linodes/configs'; | ||
| import { vpcQueryKey, subnetQueryKey } from 'src/queries/vpcs'; | ||
| import { useFlags } from 'src/hooks/useFlags'; | ||
| import { useAccount } from 'src/queries/account'; | ||
| import { isFeatureEnabled } from 'src/utilities/accountCapabilities'; | ||
|
|
||
| import { getVPCsFromLinodeConfigs } from './utils'; | ||
|
|
||
| interface Props { | ||
| linodeId: number | undefined; | ||
| onClose: () => void; | ||
|
|
@@ -17,13 +26,28 @@ interface Props { | |
| } | ||
|
|
||
| export const DeleteLinodeDialog = (props: Props) => { | ||
| const queryClient = useQueryClient(); | ||
| const flags = useFlags(); | ||
| const { data: account } = useAccount(); | ||
|
|
||
| const enableVPCActions = isFeatureEnabled( | ||
| 'VPCs', | ||
| Boolean(flags.vpc), | ||
| account?.capabilities ?? [] | ||
| ); | ||
|
|
||
| const { linodeId, onClose, onSuccess, open } = props; | ||
|
|
||
| const { data: linode } = useLinodeQuery( | ||
| linodeId ?? -1, | ||
| linodeId !== undefined && open | ||
| ); | ||
|
|
||
| const { data: configs } = useAllLinodeConfigsQuery( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR to include a VPC column in the LInodes Landing page table caused a bunch of 429s due to the useAllLinodeConfigsQuery being fired for every single linode. (see prs #9485 and #9625 for context + their respective tickets) I think that would be less of an issue here bc we only fire this query when we are deleting the linode (and also only if the user has VPC capabilities), not for every single linode, but please lmk if I'm mistaken!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is reasonable π
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you are correct! This is good! |
||
| linodeId ?? -1, | ||
| linodeId !== undefined && open && enableVPCActions | ||
| ); | ||
|
|
||
| const { error, isLoading, mutateAsync, reset } = useDeleteLinodeMutation( | ||
| linodeId ?? -1 | ||
| ); | ||
|
|
@@ -36,6 +60,24 @@ export const DeleteLinodeDialog = (props: Props) => { | |
|
|
||
| const onDelete = async () => { | ||
| await mutateAsync(); | ||
| const vpcIds = enableVPCActions | ||
| ? getVPCsFromLinodeConfigs(configs ?? []) | ||
| : []; | ||
| // @TODO VPC: potentially revisit using the linodeEventsHandler in linode/events.ts to invalidate queries rather than here | ||
| // See PR #9814 for more details | ||
| if (vpcIds.length > 0) { | ||
| queryClient.invalidateQueries([vpcQueryKey, 'paginated']); | ||
| // invalidate data for specific vpcs this linode is assigned to | ||
| vpcIds.forEach((vpcId) => { | ||
| queryClient.invalidateQueries([vpcQueryKey, 'vpc', vpcId]); | ||
| queryClient.invalidateQueries([ | ||
| vpcQueryKey, | ||
| 'vpc', | ||
| vpcId, | ||
| subnetQueryKey, | ||
| ]); | ||
| }); | ||
| } | ||
coliu-akamai marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| onClose(); | ||
| resetEventsPolling(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,9 @@ | ||
| import { parseMaintenanceStartTime } from './utils'; | ||
| import { parseMaintenanceStartTime, getVPCsFromLinodeConfigs } from './utils'; | ||
| import { | ||
| configFactory, | ||
| LinodeConfigInterfaceFactory, | ||
| LinodeConfigInterfaceFactoryWithVPC, | ||
| } from 'src/factories'; | ||
|
|
||
| describe('Linode Landing Utilites', () => { | ||
| it('should return "Maintenance Window Unknown" for invalid dates', () => { | ||
|
|
@@ -26,4 +31,39 @@ describe('Linode Landing Utilites', () => { | |
| expect(parseMaintenanceStartTime(undefined)).toBe('No Maintenance Needed'); | ||
| expect(parseMaintenanceStartTime(null)).toBe('No Maintenance Needed'); | ||
| }); | ||
|
|
||
| describe('getVPCsFromLinodeConfigs', () => { | ||
| const vpcInterfaceList = LinodeConfigInterfaceFactoryWithVPC.buildList(2); | ||
|
|
||
| it('returns an empty list if there are no vpc interfaces in the configs', () => { | ||
| const configs = configFactory.buildList(3); | ||
| const vpcIds = getVPCsFromLinodeConfigs(configs); | ||
| expect(vpcIds).toEqual([]); | ||
| }); | ||
|
|
||
| it('returns the IDs of vpc-related interfaces', () => { | ||
| const config = configFactory.build({ | ||
| interfaces: [ | ||
| ...vpcInterfaceList, | ||
| ...LinodeConfigInterfaceFactory.buildList(4), | ||
| ], | ||
| }); | ||
| const vpcIds = getVPCsFromLinodeConfigs([ | ||
| ...configFactory.buildList(3), | ||
| config, | ||
| ]); | ||
| expect(vpcIds).toEqual([2, 3]); | ||
| }); | ||
|
|
||
| it('returns unique vpc ids (no duplicates)', () => { | ||
| const vpcInterface = LinodeConfigInterfaceFactoryWithVPC.build({ | ||
| vpc_id: 2, | ||
| }); | ||
| const config = configFactory.build({ | ||
| interfaces: [...vpcInterfaceList, vpcInterface], | ||
| }); | ||
| const vpcIds = getVPCsFromLinodeConfigs([config]); | ||
| expect(vpcIds).toEqual([2, 3]); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. π |
||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -189,6 +189,7 @@ const VPCDetail = () => { | |
| {numLinodes > 0 && ( | ||
| <DismissibleBanner | ||
| preferenceKey={`reboot-linodes-warning-banner-${vpc.id}`} | ||
| sx={{ marginBottom: theme.spacing(2) }} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see PR description for reasoning behind this additional margin |
||
| variant="warning" | ||
| > | ||
| <Typography variant="body1"> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Adding this check to determine when to fire the useAllLinodeConfigsQuery (line 49) -- feature flagging the query, basically