-
Notifications
You must be signed in to change notification settings - Fork 703
Allow connect on Http2ProtocolOptions when Websockets are enabled #6846
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: main
Are you sure you want to change the base?
Conversation
45da937 to
98bfa41
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6846 +/- ##
==========================================
- Coverage 81.85% 81.67% -0.18%
==========================================
Files 130 130
Lines 15747 15750 +3
==========================================
- Hits 12889 12864 -25
Misses 2574 2574
- Partials 284 312 +28
🚀 New features to boost your workflow:
|
|
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
|
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
|
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
|
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
|
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
sunjayBhatia
left a comment
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.
Looks like we need some more unit testing to validate this config update logic
internal/envoy/v3/listener.go
Outdated
| } | ||
|
|
||
| // Assign http2Options only if it has been modified | ||
| if b.http2MaxConcurrentStreams != nil { |
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 dont think this is quite right? if max concurrent streams is not set, but websockets are enabled we would still want to set the http2 options
|
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
|
any progress on this PR? thanks |
|
Will update this week. Missed changing it for the given feedback. |
|
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
|
Hi, @rajatvig just checking in to see how the PR is progressing. |
|
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
Signed-off-by: Rajat Vig <[email protected]>
Signed-off-by: Rajat Vig <[email protected]>
Signed-off-by: Rajat Vig <[email protected]>
0da4d80 to
940ecd6
Compare
Signed-off-by: Rajat Vig <[email protected]>
940ecd6 to
fc9ef7d
Compare
|
@John-Lin @sunjayBhatia Sorry for the delay here. Rebased the branch and addressed feedback and also added unit tests for it. |
|
Am unsure on why the coverage fell after adding in unit tests. Local run shows me coverage is better |
Signed-off-by: Rajat Vig <[email protected]>
|
The Contour project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to the #contour channel in the Kubernetes Slack |
This should possibly address part of #3371 and possibly knative/serving#13083 when using Knative with Contour.
Signed-off-by: Rajat Vig [email protected]