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

upcoming: [M3-7927] - Linode Create Refactor - Part 6 - Add-ons #10319

Merged

Conversation

bnussman-akamai
Copy link
Contributor

@bnussman-akamai bnussman-akamai commented Mar 26, 2024

Description πŸ“

Adds the "Add-ons" section to the new Linode Create Flow πŸŽ‰

Changes πŸ”„

  • Adds the "Add-ons" section to the new Linode Create flow
    • The ability to add a private IP πŸ”Œ
    • The ability to enable backups πŸ’Ύ
  • Adds jsdoc style comments to the Linode Create type in api-v4 πŸ“ (copy paste from API spec)
  • Allows firewall_id to be null in the Linode Create type because the API allows it βœ…

Preview πŸ“·

Screenshot 2024-03-26 at 4 58 24β€―PM

How to test πŸ§ͺ

Prerequisites

  • Turn on the new Linode Create Flow feature flag using your local dev tools 🎏 (you can use the preview link or checkout this PR)

Verification steps

  • Go to the Linode Create Flow
  • Test general create flow functionality
  • Test both the Private IP checkbox and the Backups checkbox β˜‘οΈ
  • Check my code for anything unreadable or questionable πŸ“–
  • Verify unit tests are sufficient πŸ§ͺ

As an Author I have considered πŸ€”

  • πŸ‘€ Doing a self review
  • ❔ Our contribution guidelines
  • 🀏 Splitting feature into small PRs
  • βž• Adding a changeset
  • πŸ§ͺ Providing/Improving test coverage
  • πŸ” Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • πŸ‘£ Providing comprehensive reproduction steps
  • πŸ“‘ Providing or updating our documentation
  • πŸ•› Scheduling a pair reviewing session
  • πŸ“± Providing mobile support
  • β™Ώ Providing accessibility support

Comment on lines +29 to +34
{isEdgeRegionSelected && (
<Notice
text="Backups and Private IP are currently not available for Edge regions."
variant="warning"
/>
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are other notices that will need to be added here related to cloning. I will add those when I add cloning support down the road.

@bnussman-akamai bnussman-akamai marked this pull request as ready for review March 26, 2024 21:09
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner March 26, 2024 21:09
Copy link

github-actions bot commented Mar 26, 2024

Coverage Report: βœ…
Base Coverage: 81.69%
Current Coverage: 81.69%

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for all the doc improvements and tests. This is looking great πŸš€

@jaalah-akamai jaalah-akamai added the Add'tl Approval Needed Waiting on another approval! label Mar 27, 2024
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.

Disabled flow for restricted user w/o Linode Create permissions, functional for unrestricted users βœ…

Add-Ons section disabled when Edge region selected βœ…

Code review βœ…

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Nice and super well tested πŸŽ‰

  • Confirmed behavior of the form and payload βœ…
  • Confirmed a null firewall_id is taken by the API βœ…

Left a couple non blocking comments

Also, reminder to add the default values to the distro and details label

</Stack>
<Typography>
{isAccountBackupsEnabled ? (
<React.Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

are we still doing fragments this way rather than <>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just copy paste from packages/manager/src/features/Linodes/LinodesCreate/AddonsPanel.tsx. I'm open to using <> if we want to prefer that

type,
});

const selectedRegion = regions?.find((r) => r.id === regionId);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am still finding this a bit flawed. Yes, this is relatively inexpensive but because of only storing the region ID in the form state instead of the whole region object we are probably running regions.find 5 or 6 times on the Linode create flow - maybe more. Not a big deal but not the cleanest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is a great point. I was considering adding a useRegion query to avoid needing to run a regions.find everywhere, but this could introduce an extra fetch.

This might be worth it considering how often we call regions.find throughout the app

Have any thoughts on that or other ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible/a good idea to break pattern and have the form state being the whole region?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that would be great. I think it would make things a ton more complicated with regarding types and validation

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense. then i think the best option would be a useMemo util so at least we're not recalculating every time?

Co-authored-by: Dajahi Wiley <114682940+dwiley-akamai@users.noreply.github.com>
@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Mar 27, 2024
@bnussman-akamai bnussman-akamai merged commit d4e0d0a into linode:develop Mar 27, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Linode Create Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants