Allow connect on Http2ProtocolOptions when Websockets are enabled#6846
Allow connect on Http2ProtocolOptions when Websockets are enabled#6846rajatvig wants to merge 5 commits intoprojectcontour:mainfrom
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.
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.
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 |
|
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]