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: Add additional power field types for IP address, LXD, and Virsh #5447

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ndv99
Copy link
Contributor

@ndv99 ndv99 commented May 29, 2024

Done

  • Added LXD, Virsh, and IP address to power field types (in line with new backend types)
  • Added simple IP address validaiton (check if field value is a valid IP address)

QA steps

  • Go to a machine with an [IPMI / LXD / Virsh] power type (details page, "Configuration")
  • Click "Edit" under "Power configuration"
  • Ensure the IP address field is displayed correctly
  • Type an invalid IP address into that field and press tab
  • Ensure an error is displayed

Notes

The MP for the backend implementation of this is here

@ndv99 ndv99 added Review: Code needed Review: Core needed QA is required from the MAAS core team labels May 29, 2024
@webteam-app
Copy link

@ndv99
Copy link
Contributor Author

ndv99 commented May 29, 2024

Hm, I'm getting some real strange behavior in tests surrounding a couple of selectors

src/app/subnets/views/VLANDetails/VLANDetails.tsx:39:19

const subnets = useSelector((state: RootState) =>
    subnetSelectors.getByIds(state, vlan?.subnet_ids || [])
  );

src/app/machines/views/MachineDetails/MachineConfiguration/TagForm/TagForm.tsx:28:18

 const errors = useSelector((state: RootState) =>
    machineSelectors.eventErrorsForIds(state, systemId, [
      NodeActions.TAG,
      NodeActions.UNTAG,
    ])
  )[0]?.error;

Not quite sure what's causing these (or especially how it's related to this PR's changes), @petermakowski could you take a look if you have a minute?

@petermakowski
Copy link
Collaborator

Not quite sure what's causing these (or especially how it's related to this PR's changes), @petermakowski could you take a look if you have a minute?

#5448

Copy link
Collaborator

@petermakowski petermakowski left a comment

Choose a reason for hiding this comment

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

Have you checked the bundle size impact of adding a new dependency?

Comment on lines +70 to +78
: field.field_type === PowerFieldType.IP_ADDRESS ||
field.field_type === PowerFieldType.VIRSH_ADDRESS ||
field.field_type === PowerFieldType.LXD_ADDRESS
? Yup.string().test({
name: "is-ip-address",
message: "Please enter a valid IP address.",
test: (value) => isIP(value as string),
})
: Yup.string();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function has become quite difficult to read. I'd suggest to move this logic into a separate function, e.g. getPowerFieldSchema that takes PowerField and returns correct Yup schema.

@ndv99
Copy link
Contributor Author

ndv99 commented May 29, 2024

Have you checked the bundle size impact of adding a new dependency?

Good point - I'll run a before/after check. Although likelihood is we'll need this anyway for upcoming reserved IP work

@ndv99
Copy link
Contributor Author

ndv99 commented May 29, 2024

Have you checked the bundle size impact of adding a new dependency?

Good point - I'll run a before/after check. Although likelihood is we'll need this anyway for upcoming reserved IP work

image

Adding is-ip increases the (compressed) bundle size by 4066KiB

@ndv99 ndv99 force-pushed the feat-power-field-ip-validation branch from 86aea4e to c59e8b8 Compare May 29, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: Code needed Review: Core needed QA is required from the MAAS core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants