-
Notifications
You must be signed in to change notification settings - Fork 367
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-8869] - Update types for NodeBalancer UDP support #11321
upcoming: [M3-8869] - Update types for NodeBalancer UDP support #11321
Conversation
Coverage Report: ✅ |
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.
Code review ✅
No regressions observed to existing flow ✅
re: mode
and how it shouldn't be specified when creating a node for a UDP NodeBalancer -- should this be reflected in L54 of nodebalancers.schema.ts
?
packages/manager/.changeset/pr-11321-tech-stories-1733243149867.md
Outdated
Show resolved
Hide resolved
const client_conn_throttle = number() | ||
.min( | ||
CONNECTION_THROTTLE.MIN, | ||
`Client Connection Throttle must be between ${CONNECTION_THROTTLE.MIN} and ${CONNECTION_THROTTLE.MAX}.` | ||
) | ||
.max( | ||
CONNECTION_THROTTLE.MAX, | ||
`Client Connection Throttle must be between ${CONNECTION_THROTTLE.MIN} and ${CONNECTION_THROTTLE.MAX}.` | ||
) | ||
.typeError('Client Connection Throttle must be a number.'); | ||
|
||
const client_udp_sess_throttle = number() | ||
.min( | ||
CONNECTION_THROTTLE.MIN, | ||
`UDP Session Throttle must be between ${CONNECTION_THROTTLE.MIN} and ${CONNECTION_THROTTLE.MAX}.` | ||
) | ||
.max( | ||
CONNECTION_THROTTLE.MAX, | ||
`UDP Session Throttle must be between ${CONNECTION_THROTTLE.MIN} and ${CONNECTION_THROTTLE.MAX}.` | ||
) | ||
.typeError('UDP Session Throttle must be a number.'); |
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.
Can put the error messages that are used twice in their own constants
@@ -116,7 +125,12 @@ export const createNodeBalancerConfigSchema = object({ | |||
then: (schema) => | |||
schema.required('SSL certificate is required when using HTTPS.'), | |||
}), | |||
stickiness: mixed().oneOf(['none', 'table', 'http_cookie']), | |||
stickiness: string().when('protocol', { |
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.
Should we also check the following fields when the protocol is udp
?
check_passive
must befalse
or unsetproxy_protocol
must benone
or unset- The various SSL related fields like
ssl_cert
,ssl_key
,cipher_suite_recommended
should not be set mode
should not be specified when creating a node
Cloud Manager UI test results🎉 466 passing tests on test run #12 ↗︎
|
Cloud Manager E2E Run #6938
Run Properties:
|
Project |
Cloud Manager E2E
|
Branch Review |
develop
|
Run status |
Failed #6938
|
Run duration | 28m 26s |
Commit |
85d62d3469: upcoming: [M3-8869] - Update types for NodeBalancer UDP support (#11321)
|
Committer | Banks Nussman |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
1
|
Flaky |
3
|
Pending |
2
|
Skipped |
0
|
Passing |
465
|
View all changes introduced in this branch ↗︎ |
Tests for review
cypress/e2e/core/billing/smoke-billing-activity.spec.ts • 1 failed test
Test | Artifacts | |
---|---|---|
Billing Activity Feed > displays correct timezone for invoice and payment dates |
Screenshots
Video
|
linodes/switch-linode-state.spec.ts • 1 flaky test
Test | Artifacts | |
---|---|---|
switch linode state > powers off a linode from landing page |
Screenshots
Video
|
linodes/update-linode-labels.spec.ts • 1 flaky test
Test | Artifacts | |
---|---|---|
update linode label > updates a linode label from the "Settings" tab |
Screenshots
Video
|
volumes/create-volume.smoke.spec.ts • 1 flaky test
Test | Artifacts | |
---|---|---|
volumes > does not allow creation of a volume with invalid pricing from volumes landing |
Screenshots
Video
|
Description 📝
Preview 📷
Note
No UI changes
How to test 🧪
M3-8869
)As an Author, I have considered 🤔
As an Author, before moving this PR from Draft to Open, I confirmed ✅