Skip to content
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: [M3-6755] - Unassign Linodes from Subnet #9703

Merged
merged 12 commits into from
Oct 2, 2023

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Sep 20, 2023

Description 📝

Allow for un-assigning Linodes from subnets.

Major Changes 🔄

  • Allow for Linodes can be batch unassigned
  • Add unassigned Linodes to CSV in order to provide list for instances that need to be rebooted

Preview

Screenshots Video
Screenshot 2023-09-26 at 1 42 39 PM
unassign.mp4

How to test 🧪

Note
Unit / Integration Tests will be handled in a separate PR [M3-6771]

  1. Ensure you have both customer tags applied (see me for details)
  2. Ensure feature flag is enabled in environment
  3. Point env variables to DEV and use incognito to hit local host
  4. Go to VPC > Subnets and ensure you have some Linode configs in your subnet (if not, add them)
  5. Otherwise, click actions menu "..." to Unassign Linodes
  6. Selecting a linode config should show "Linodes to be Unassigned from Subnet (x)" with a list of Linodes selecting
    1. You should be able to undo selections in multiple ways via the UI
  7. Downloading the CSV (no linodes assigned, 1+ linodes assigned, determine that CSV accurately reflects the linodes in the 'Linodes assigned to Subnet (x) list) (should have linode's ID, label, public ipv4 at minimum, may include more fields)
  8. Double check that you can't unassign Linodes to a subnet if you don't have the permissions for that specific vpc

@jaalah-akamai jaalah-akamai requested a review from a team as a code owner September 20, 2023 03:33
@jaalah-akamai jaalah-akamai self-assigned this Sep 20, 2023
@jaalah-akamai jaalah-akamai requested review from cpathipa, cliu-akamai, dwiley-akamai and hana-akamai and removed request for a team September 20, 2023 03:33
@jaalah-akamai jaalah-akamai marked this pull request as draft September 20, 2023 03:40
@dwiley-akamai dwiley-akamai added the VPC Relating to VPC project label Sep 20, 2023
@jaalah-akamai
Copy link
Contributor Author

jaalah-akamai commented Sep 20, 2023

I'll look into the CSV issue. In the meantime, I'm waiting on @coliu-akamai 's PR #9687 before continuing work here since there's a lot of overlap between us

@jaalah-akamai jaalah-akamai changed the title WIP feat: [M3-6755] - Unassign Linodes from Subnet feat: [M3-6755] - Unassign Linodes from Subnet Sep 26, 2023
@jaalah-akamai jaalah-akamai marked this pull request as ready for review September 26, 2023 18:02
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functionality is looking good from my initial testing 🙌

Comment on lines +126 to +144
const response = await queryClient.fetchQuery(
[linodesQueryKey, 'linode', linode.id, 'configs'],
() => getAllLinodeConfigs(linode.id)
);

if (response) {
const configWithVpcInterface = response.find((config) =>
config.interfaces.some(
(_interface) =>
_interface.subnet_id === subnet?.id &&
_interface.purpose === 'vpc'
)
);

const vpcInterface = configWithVpcInterface?.interfaces?.find(
(_interface) =>
_interface.subnet_id === subnet?.id &&
_interface.purpose === 'vpc'
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be able to do something similar to what I did in

let _configInterfaceWithVPC: Interface | undefined;
// eslint-disable-next-line no-unused-expressions
configs?.find((config) => {
const interfaces = config.interfaces;
const interfaceWithVPC = interfaces.find(
(_interface) => _interface.vpc_id === vpcLinodeIsAssignedTo?.id
);
if (interfaceWithVPC) {
_configInterfaceWithVPC = interfaceWithVPC;
}
return interfaceWithVPC;
});

to avoid doing the same _interface logic twice

@cpathipa
Copy link
Contributor

cpathipa commented Sep 27, 2023

Initial observation:The drawer matches the mockup, but I feel like it is a bit inconsistent with other drawers where we attach Linode's, for example, volumes. In those cases, action buttons are shown, but here they are hidden until Linode's are selected from the dropdown. Can we confirm with UX if this is an intentional design change?
image

IMO, Action buttons should be show here like other drawers.

image

Additionally, on smaller screens without action buttons, it seems there is no way to navigate back.

image

Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality LGMT! Other than action buttons not being shown in drawer.
Agree with @hana-linode and @dwiley-akamai feedback.

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit tests pass ✅
CSV looks as expected ✅
Unassigning single and multiple Linodes ✅
Restricted users with no edit permissions on the VPC can't initiate unassigns ✅

The functionality is looking good to me -- going ahead and approving but there's one open question on how to best handle the errors

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ unit tests pass
✅ csv looks as expected
✅ unassign single linode
✅ unassign multiple linodes ***
✅ almost forgot, restricting permissions

** I'm still getting the autocomplete warning if I click 'select all' and have 2+ linodes selected to unassign from the subnet (this warning will not appear if we click 'select all' but only have one linode). Might be something to do with the autocomplete not rerendering until after we've unassigned all the linodes -- should we move this issue to investigate it in a separate ticket?

image

@hana-akamai
Copy link
Contributor

@coliu-akamai Yep, created M3-7207 to look into this

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving -- functionality works as expected + follow up ticket made. Awesome job! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for Review VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants