-
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
upcoming: [M3-7840] - Placement Groups Query Key Factory #10314
Conversation
queryClient.invalidateQueries(placementGroupQueries.paginated._def); | ||
queryClient.invalidateQueries(placementGroupQueries.all._def); | ||
queryClient.setQueryData<PlacementGroup>( | ||
placementGroupQueries.placementGroup(placementGroup.id).queryKey, |
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.
when creating a new PG, we're really only concerned about the Placement Group queries since at the time of creation the PG will be empty
queryClient.invalidateQueries(placementGroupQueries.all._def); | ||
queryClient.removeQueries( | ||
placementGroupQueries.placementGroup(id).queryKey | ||
); |
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 can't delete a PG with Linodes in it, so at the time of deletion, it is empty therefore no need to invalidate any Linode
); | ||
|
||
// Invalidate all linodes query since we use the list to populate the select in the assign drawer | ||
queryClient.invalidateQueries([linodeQueryKey, 'all']); |
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.
For assign and unassign, we're only worried about invalidating the allLinodesQuery because that's the only relationship query we use at the moment (for the PG select).
There is no representation of the PG in the linode details UI as it stands
queryClient.setQueryData( | ||
[queryKey, 'placement-group', id], | ||
placementGroupQueries.placementGroup(id).queryKey, |
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.
When editing a PG, we're only editing its label, so no need to invalidate any Linode query here since the PG is not represented in the Linode details UI
Coverage Report: ✅ |
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.
Overall LGTM!
- Confirming on creating PG.
- Confirming on editing PG.
- Confirming on deleting PG.
- Confirming on creating Linode with PG.
Observations (Please ignore if these are under randar or yet to implement).
-
Linodes created with PG are not shown under PG table. Also, when no linodes are assigned showing empty tooltip with language "0 of "is confusing.
-
The language indicating that Linodes have been assigned to this placement group doesn't seem relevant when no Linodes are assigned.
-
The extra whitespace between the horizontal line appears somewhat odd to me. Additionally, the region and affinity information seems to be grouped with the title.
@cpathipa thanks for bringing all these points. It's hard to keep PR descriptions updated with work in progress but all points you mentioned are being addressed in subsequent PRs (and some of them like the missing linode count is the API not returning the right data yet). I'll try to add more context in future PRs |
Description 📝
Small PR to being key factory to placement groups and update some naming conventions. Please see the self review for more contextual info about about the invalidation choices (or lack thereof).
Changes 🔄
useUnpaginatedPlacementGroupsQuery
withuseAllPlacementGroupsQuery
Preview 📷
No change/regression to the UI is to be expected with this PR
How to test 🧪
Prerequisites
Verification steps
As an Author I have considered 🤔
Check all that apply