-
Notifications
You must be signed in to change notification settings - Fork 365
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-7726] - Assign Linodes to Placement Group Drawer #10140
Conversation
aef2fe6
to
f32a472
Compare
CreatePlacementGroupPayload, | ||
PlacementGroup, | ||
UnassignVMsFromPlacementGroupPayload, | ||
UnassignLinodesFromPlacementGroupPayload, |
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've updated utils to read "linode(s)" instead of "VM(s)" for consistency since VM is more of of product name and linode is more clear in code
/** | ||
* Additional styles to apply to the component | ||
*/ | ||
sx?: SxProps<Theme>; |
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.
Improving the RemovableSelectionList
to make it a tad more flexible
|
||
export const unassignVMsFromPlacementGroupSchema = object({ | ||
linodeIds: array().of(number().max(1, 'Only one Linode id is supported.')), | ||
}); |
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 don't really need those schemas cause we're using a drawer that does requests outside of a regular formik/form schema. We still have validation, but we don't need a schema for the one field
'linode', | ||
updatedPlacementGroup.linode_ids[0], | ||
'placement_groups', | ||
]); |
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 more granularity to our invalidations
Coverage Report: ✅ |
8fb5179
to
d75f5f6
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.
return [...acc, ...placementGroup.linode_ids]; | ||
}, []); | ||
|
||
return Array.from(new Set(linodeIds)); |
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.
Nice
packages/manager/src/features/PlacementGroups/PlacementGroupsAssignLinodesDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/PlacementGroups/PlacementGroupsAssignLinodesDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/PlacementGroups/PlacementGroupsAssignLinodesDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/PlacementGroups/PlacementGroupsAssignLinodesDrawer.tsx
Show resolved
Hide resolved
@jaalah-akamai yes the banner is expected - i intentionally made it appear with our mocks upon reaching the 10 limit in the drawer
So the permissions are the same as the Linode permissions per the API specs. Not sure what the approach should be. We have a separate ticket to handle those |
packages/manager/src/features/PlacementGroups/PlacementGroupsAssignLinodesDrawer.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.
Verified form behavior and adding/removing a Linode from the Placement Group
Description 📝
This PR brings the drawer to assign linodes to a PG (in the VPC subnet fashion) + improvements and naming convention updates
Changes 🔄
Preview 📷
How to test 🧪
Prerequisites
Verification steps
NOTE: due to limitations in using MSW, we can only really see adding/removing ONE linode at a time (subsequent tries won't be reflected in the UI)
As an Author I have considered 🤔
Check all that apply