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-9141] - Add udp_check_port support to NodeBalancers #11534

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Jan 17, 2025

Description 📝

  • Adds support for the new udp_check_port NodeBalancer config field ➕
  • Updated UI to minimize layout shifting when changing protocol and other fields 🎨
  • Removed the "HTTP Cookie" session stickiness option when the tcp protocol is selected 🚫
  • Moved required fields to the top of the "Active HealthChecks" section ⬆️

Preview 📷

Before After
New field Screenshot 2025-01-17 at 12 59 23 PM Screenshot 2025-01-17 at 12 59 03 PM
Updated TCP layout Screenshot 2025-01-17 at 1 01 30 PM Screenshot 2025-01-17 at 1 01 20 PM
Updated HTTPS layout Screenshot 2025-01-17 at 1 02 26 PM Screenshot 2025-01-17 at 1 02 34 PM

How to test 🧪

  • Use dev API
  • Ensure the udp flag is enabled
  • Ensure you have the nb-udp-alpha customer tag
  • Test the new "Health Check Port" field
    • Protocol must be UDP for this new field to be visible
  • Verify the UI changes have improve the layout shifting and overall user experience
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


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@bnussman-akamai bnussman-akamai added the NodeBalancers Relating to NodeBalancers label Jan 17, 2025
@bnussman-akamai bnussman-akamai self-assigned this Jan 17, 2025
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner January 17, 2025 18:03
@bnussman-akamai bnussman-akamai requested review from hana-akamai and cpathipa and removed request for a team January 17, 2025 18:03
@bnussman-akamai bnussman-akamai changed the title upcoming: [M3-9141] - Add udp_check_port to NodeBalancers upcoming: [M3-9141] - Add udp_check_port support to NodeBalancers Jan 17, 2025
Copy link

github-actions bot commented Jan 17, 2025

Coverage Report:
Base Coverage: 79.11%
Current Coverage: 79.11%

@bnussman-akamai bnussman-akamai requested a review from a team as a code owner January 17, 2025 19:44
@bnussman-akamai bnussman-akamai requested review from jdamore-linode and removed request for a team January 17, 2025 19:44
Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Dev isn't loading for me rn but the code is looking good

Can we make the Interval and Timeout inputs smaller so there's more spacing between columns?

image

@bnussman-akamai
Copy link
Member Author

@hana-akamai I honestly don't know how to do that with MUI grid. Does this look any better?

Screenshot 2025-01-21 at 6 20 39 PM

@hana-akamai
Copy link
Contributor

@bnussman hmm looks a bit too cramped with 3 side by side, I guess we can keep it as is

I'm getting an error when trying to save the UDP changes, is this expected?

image

@bnussman-akamai
Copy link
Member Author

bnussman-akamai commented Jan 22, 2025

@hana-akamai I wasn't able to replicate that. You'll need the nb-udp-alpha customer tag for the API request to go through. I recommend using dev if possible. Sorry for not including that in the description!

@hana-akamai hana-akamai added the Add'tl Approval Needed Waiting on another approval! label Jan 23, 2025
@bnussman-akamai bnussman-akamai removed the Add'tl Approval Needed Waiting on another approval! label Jan 24, 2025
@bnussman-akamai bnussman-akamai added the Approved Multiple approvals and ready to merge! label Jan 24, 2025
@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 1 failing test on test run #17 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
1 Failing481 Passing2 Skipped98m 36s

Details

Failing Tests
SpecTest
linode-config.spec.tsEnd-to-End » Clones a config

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/linodes/linode-config.spec.ts"

@bnussman-akamai bnussman-akamai merged commit cc51ff4 into linode:develop Jan 24, 2025
22 of 23 checks passed
Copy link

cypress bot commented Jan 24, 2025

Cloud Manager E2E    Run #7126

Run Properties:  status check passed Passed #7126  •  git commit cc51ff4dc7: upcoming: [M3-9141] - Add `udp_check_port` support to NodeBalancers (#11534)
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #7126
Run duration 30m 22s
Commit git commit cc51ff4dc7: upcoming: [M3-9141] - Add `udp_check_port` support to NodeBalancers (#11534)
Committer Banks Nussman
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 4
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 482
View all changes introduced in this branch ↗︎

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! NodeBalancers Relating to NodeBalancers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants