feat: [M3-7031] - Add AGLB Create Service Target Drawer by bnussman-akamai · Pull Request #9657 · linode/manager

bnussman-akamai · 2023-09-08T17:31:59Z

Description 📝

Adds the AGLB Create Service Target Drawer 🎉

Preview 📷

How to test 🧪

Check out this PR
Turn on the MSW
Go to http://localhost:3000/loadbalancers/0/service-targets
Click Create Service Target
Test the drawer 🔍
- The Linode Select and Certificate Select both are infinite scrollable and API filterable so your mileage will vary on the MSW

abailly-akamai

Forms looks and feels great!

Added a few comments for when we are revisiting. Also wasn't able to to filter/search results in the LinodeOrIp component, which i dunno if to be expected or not. Self review would be appreciated for components that may not feature full functionality if so.

...es/manager/src/features/LoadBalancers/LoadBalancerDetail/ServiceTargets/LinodeOrIPSelect.tsx

abailly-akamai · 2023-09-12T14:15:43Z

@bnussman can we also add test coverage for new components?

bnussman-akamai · 2023-09-12T15:03:58Z

@abailly-akamai Searching and filtering the LinodeOrIP select will not work with mocks because it is API filtered. If you want to test it, you can leave the MSW on but comment out the */linode/instances endpoint in serverHandlers.ts

abailly-akamai

Looking great - added some more comments

packages/manager/src/components/TextField.tsx

...ges/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/CertificateSelect.tsx

...ges/manager/src/features/LoadBalancers/LoadBalancerDetail/ServiceTargets/AddEndpointForm.tsx

.../features/LoadBalancers/LoadBalancerDetail/ServiceTargets/CreateServiceTargetDrawer.test.tsx

...ges/manager/src/features/LoadBalancers/LoadBalancerDetail/ServiceTargets/AddEndpointForm.tsx

packages/manager/src/foundations/themes/dark.ts

packages/manager/src/foundations/themes/light.ts

abailly-akamai

Awesome work on this, well broken down and tested (some components covered by e2e as discussed i the comments)

Approving on second pass have not found any issue but left a couple comments to confirm some behaviours.

abailly-akamai · 2023-09-20T14:52:59Z

packages/manager/src/features/Events/eventMessageGenerator.ts

-      `A device has been removed from Firewall ${e.entity?.label ?? ''}.`,
+      `A ${e.secondary_entity?.type} has been removed from Firewall ${
+        e.entity?.label ?? ''
+      }.`,


Should you also consider the fallback for an undefined secondary entity here?

Oops. I did not mean to commit this 😖 Going to revert. This work is covered by #9693

abailly-akamai · 2023-09-20T14:54:56Z

...ges/manager/src/features/LoadBalancers/LoadBalancerDetail/ServiceTargets/AddEndpointForm.tsx

+      </Button>
+    </>
+  );
+};


This may be related to mock data, but should we be able to add the same endpoint several times and for it not to be filtered out after being added in the table?

I took note to follow up and see how the API will validate duplicate endpoints

jdamore-linode

Looks great @bnussman-akamai! Confirmed that the create service target drawer works as expected as much as possible with MSW enabled

mjac0bs

Drawer looked good! Let's just fix the typo in the algorithm options.

Also called out a few places where we have TODOs and based on our past team discussions, it would be great to include the project name (and ticket number if we have it).

...r/src/features/LoadBalancers/LoadBalancerDetail/ServiceTargets/CreateServiceTargetDrawer.tsx

carrillo-erik · 2023-09-20T18:52:57Z

When entering a very long name for Host it causes the close icon to be pushed out of the view. Would it be possible to make the text in the middle column to wrap at a lower value to decrease to avoid this?

carrillo-erik · 2023-09-20T19:09:16Z

@bnussman

Upon creating a service target and receiving the response for the POST request, Cloud doesn't update the service targets list with the newly created service target from what I could see -- is that outside the scope of this PR?

Any follow up on this? I noticed the comment is about a week old and I was able to make the same observation just now.

carrillo-erik · 2023-09-20T19:13:46Z

...ges/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/CertificateSelect.tsx

@@ -75,17 +77,13 @@ export const CertificateSelect = (props: Props) => {
          setInputValue(value);


Is there a specific reason why this logic is separated from the logic in the onChange in line 84?

This is because we need to track the inputValue separately in state because we use it to API filter the certificates.

Regarding that other comment, that is just how the MSW and React Query work together. When we have a real API to test against, things should work!

initial drawer

24e41bd

bnussman-akamai added the ACLB Relating to the Akamai Cloud Load Balancer label Sep 8, 2023

bnussman-akamai self-assigned this Sep 8, 2023

bnussman added 13 commits September 8, 2023 13:32

start building healthchecks

14be7e8

finish health check inputs

03af11f

add client side validation

d6605e5

fix healthcheck validation

3f18031

add most robust error handling

12ae865

wire up goofy cert select

c078649

fix weird style

e37e3cd

order props

43c1b68

handle general errors

2fd6a52

make ca_certificate nullable

2f72a76

Merge branch 'develop' into M3-7031-aglb-service-target-create-drawer

41edc89

Add Linode or IP input

8094f50

add new Selects

9be3e9a

bnussman-akamai marked this pull request as ready for review September 11, 2023 16:49

Added changeset: Add AGLB Create Service Target Drawer

addeee4

bnussman-akamai requested review from abailly-akamai and cpathipa September 11, 2023 16:50

bnussman-akamai added the Ready for Review label Sep 11, 2023

abailly-akamai reviewed Sep 12, 2023

View reviewed changes

bnussman added 3 commits September 12, 2023 11:09

feedback @abailly-akamai

79cd77f

add basic unit test and minor improvments

5b11c1b

clean up and comment

9c1d7bf

jdamore-linode self-requested a review September 14, 2023 20:08

Joe D'Amore added 2 commits September 14, 2023 16:08

Add QA data attributes for service target create drawer

231c619

Add Load Balancer service target integration tests

20ccd40

abailly-akamai reviewed Sep 15, 2023

View reviewed changes

bnussman added 8 commits September 15, 2023 11:23

feedback @abailly-akamai

206ed6a

implement UX feedback

91f7b40

more feedback

e38df3c

move port to own line and improve validation

d7a2a3a

try to match mockup closer

6acad64

fix e2e now that we show linode label in table

53fe3e5

use e2e to test form

10756a6

Merge branch 'develop' into M3-7031-aglb-service-target-create-drawer

ded5ec1

bnussman-akamai requested a review from a team as a code owner September 19, 2023 16:53

bnussman-akamai requested review from abailly-akamai, carrillo-erik, jdamore-linode and mjac0bs and removed request for a team September 19, 2023 16:53

abailly-akamai approved these changes Sep 20, 2023

View reviewed changes

bnussman added 2 commits September 20, 2023 11:17

Merge branch 'develop' into M3-7031-aglb-service-target-create-drawer

0bf845e

revert unintended change

3e49db7

jdamore-linode approved these changes Sep 20, 2023

View reviewed changes

mjac0bs approved these changes Sep 20, 2023

View reviewed changes

mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Sep 20, 2023

carrillo-erik approved these changes Sep 20, 2023

View reviewed changes

carrillo-erik reviewed Sep 20, 2023

View reviewed changes

feedback @mjac0bs

4098d99

bnussman-akamai merged commit 801ea9d into linode:develop Sep 21, 2023

cpathipa mentioned this pull request Sep 29, 2023

Release v1.104.0 - release → staging #9740

Merged

mjac0bs mentioned this pull request Oct 2, 2023

Release v1.104.0 - staging → master #9744

Merged

		@@ -75,17 +77,13 @@ export const CertificateSelect = (props: Props) => {
		setInputValue(value);

Conversation

bnussman-akamai commented Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Preview 📷

How to test 🧪

Uh oh!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abailly-akamai commented Sep 12, 2023

Uh oh!

bnussman-akamai commented Sep 12, 2023

Uh oh!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

abailly-akamai Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

bnussman-akamai Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

abailly-akamai Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

bnussman-akamai Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

jdamore-linode left a comment

Choose a reason for hiding this comment

Uh oh!

mjac0bs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carrillo-erik commented Sep 20, 2023

Uh oh!

carrillo-erik commented Sep 20, 2023

Uh oh!

carrillo-erik Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

bnussman-akamai Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bnussman-akamai commented Sep 8, 2023 •

edited

Loading