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-7294] - Add AGLB Routes Step to Full Create Flow #9997

Merged

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Dec 13, 2023

Description 📝

  • Adds Route support to the AGLB full create flow
  • Updates service targets to be scoped to each configuration based on UX request

Note

This PR adds Routes, but does not support rules yet. I will add rule functionality in a separate PR to keep the size down. Rules are complex because of the drag and drop.

Preview 📷

Screenshot 2023-12-13 at 12 14 06 PM

How to test 🧪

Prerequisites

  • Turn on the AGLB Full Create Flow feature flag
  • Go to http://localhost:3000/loadbalancers/create

Verification steps

  • Verify you can add, update, and remove routes on the AGLB create flow

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

@bnussman-akamai bnussman-akamai added the ACLB Relating to the Akamai Cloud Load Balancer label Dec 13, 2023
@bnussman-akamai bnussman-akamai self-assigned this Dec 13, 2023
@bnussman-akamai bnussman-akamai marked this pull request as ready for review December 13, 2023 18:35
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner December 13, 2023 18:35
@bnussman-akamai bnussman-akamai requested review from jdamore-linode and coliu-akamai and removed request for a team December 13, 2023 18:35
Copy link

github-actions bot commented Dec 13, 2023

Coverage Report:
Base Coverage: 77.96%
Current Coverage: 78%

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.

LGMT, functionality is working as expected!

  • - Verified add, update, and remove routes on the AGLB create flow.
  • - Confirmed state is updating as expected.
    image

Noticed console log error while creating service targets followed by routes.
image

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.

this looks awesome! great work Banks 🎉
✅ verified adding, removing, editing routes
✅ verified unit tests

A couple things I noticed, but they might be unrelated to this PR/out of scope:

The search bar
a) changes size whenever I start typing in it
b) a console error pops up -- changing from uncontrolled to controlled input (you can see in the video it pops up)
This is an issue on the Service Targets part too

search bar size change console error
Screen.Recording.2023-12-18.at.4.41.42.PM.mov
image


const allOtherRoutes = values.configurations.reduce<RoutePayload[]>(
(acc, configuration, configIndex) => {
const otherRoutes = configuration.routes!.filter((r, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const otherRoutes = configuration.routes!.filter((r, index) => {
const otherRoutes = configuration.routes!.filter((_, index) => {

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a very small nitpick so feel free to ignore lol

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Tested:

  • This was looking good from my testing of adding HTTP/S and TCP routes, updating, and removing routes in the AGLB full create flow, and checking for label uniqueness and validation.
  • Tests are passing.
  • Confirmed no console errors - thanks for fixing the uncontrolled input error Connie mentioned; I'd noticed that too. 🙏🏼

Question:

  • What prompted the UX request to scope service targets to each configuration?

Non-blocking:

  • Let's do a find and replace on "handleAddServiceTraget" and "handleEditServiceTraget" to correct typos. I suggested changes as I saw them reading through the code, but likely missed some.
  • Agreed that that search bar size change Connie mentioned to accommodate the 'x' looks weird. Can we set it to a constant width at the larger breakpoints?

size="small"
sx={{ padding: 'unset' }}
>
<CloseIcon sx={{ color: '#aaa !important' }} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why we have to apply the important custom styling here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was the only way I could easily achieve what the UX mockups showed. As AGLB progresses, I'll likely turn this "filter" TextField into a more reusable component. It's currently duplicated a lot 😖

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Dec 19, 2023
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.

thanks for the fixes Banks! approving pending Mariah's comments addressed 🎉

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! Ready for Review labels Dec 19, 2023
@bnussman-akamai bnussman-akamai merged commit cdef4e8 into linode:develop Dec 19, 2023
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACLB Relating to the Akamai Cloud Load Balancer Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants