-
Notifications
You must be signed in to change notification settings - Fork 374
upcoming: [M3-9114] - Add support for Linode Interfaces in Firewalls AddLinodeDrawer #12035
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
base: develop
Are you sure you want to change the base?
Conversation
e534839
to
3d81172
Compare
filterLinodes(); | ||
}, [allLinodes, assignedInterfaceIds, assignedLinodes, readOnlyLinodeIds]); |
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 am not sure what the best approach to filter out Linodes is.
Previously we could just pass in linodeOptionsFilter
to the LinodeSelect and just directly call linodes.filter(linodeOptionsFilter)
. This doesn't work anymore now that we need to fetch a Linode's interfaces to determine if it can be assigned to a firewall (updating linodeOptionsFilter
to be an async function)
I've settled on useEffect but I'm not too confident about this approach, especially bc I had to memoize a bunch of the dependencies (or run into an infinite loop 😬)
My react knowledge is lacking...anyone have other ideas? 🥲
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.
Here's my attempt at simplifying things: https://github.com/coliu-akamai/manager/compare/m3-9114...bnussman-akamai:firewall-poc?expand=1
I was able to get rid of the useEffect without any regressions (i think) and I was able to clean up overall logic a bit, although the POC it is unfinished
I didn't get error handling working and I didn't handle VLAN interfaces gracefully, but take a look and let me know what you think. If you like any of this better, you could try introducing it into your PR.
With that said, I don't want to de-rail your current progress and slow us down, so if you'd rather we refactor this as a follow up effort, just let me know.
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.
thanks Banks, will take a look! definitely interested in getting rid of the useEffect
for now since we're unsure of timelines and potentially proceeding with the out of cycle release, I'd like to plan this refactor as a follow up. However, if we hear that dates get pushed back, then I'll look into making the changes here - how does that sound?
(edit: unless if I finish my current ticket soon and have more time, will look into refactoring here)
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.
Works for me!
{isLinodeInterfacesEnabled && | ||
linodesWithMultiInterfaces.map((linode) => ( | ||
<Select |
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.
Discussed with UX - since it's very unlikely for users to have Linodes with multiple interfaces, we're ok with having these additional selects on the AddLinodeDrawer to allow for multiselecting Linodes. iirc we're also going with a single select for interfaces to reduce complexity (? will double check)
🤔 May circle back to design esp if this drawer ends up needing a lot of scrolling though... Wondering if combining something like the CreateFirewallDrawer custom vs template firewall select + the Subnet Assign drawer for interface devices could be viable
packages/manager/src/features/Firewalls/FirewallDetail/Devices/AddLinodeDrawer.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Firewalls/FirewallDetail/Devices/AddLinodeDrawer.tsx
Outdated
Show resolved
Hide resolved
interfaceLinodes.map(async (linode) => { | ||
const linodeId = linode.id; | ||
const interfaces = await getLinodeInterfaces(linodeId); | ||
// vlan interfaces cannot have a firewall assigned to them |
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.
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.
yup I think UX will have us hiding the VLAN interface firewall field eventually, will confirm + make a follow up ticket to track it and other UX feedback soon
…/AddLinodeDrawer.tsx Co-authored-by: Dajahi Wiley <[email protected]>
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 job making all of this come together 🙏
Cloud Manager UI test results🔺 1 failing test on test run #21 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/rebuild-linode.spec.ts" |
Description 📝
Changes 🔄
Target release date 🗓️
4/24 ish, 4/22 if possible
Preview 📷
create firewall drawer:

How to test 🧪
Have the customer tag + checkout this pr locally or use preview link
Verification steps
Create Firewall drawer
Author Checklists
As an Author, to speed up the review process, I 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
As an Author, before moving this PR from Draft to Open, I confirmed ✅