fix ListenerSet conformance test#8910
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 202af4ea0e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8910 +/- ##
==========================================
+ Coverage 74.87% 74.94% +0.07%
==========================================
Files 252 252
Lines 40792 40945 +153
==========================================
+ Hits 30542 30687 +145
- Misses 8168 8169 +1
- Partials 2082 2089 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdfcdd87e6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
zirain
force-pushed
the
rebase-8361
branch
4 times, most recently
from
72072f3 to
ea14a08
Compare
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| // No conditions yet means it will be set to ready during validation. | ||
| if len(conditions) == 0 { | ||
| return true | ||
| } |
There was a problem hiding this comment.
This can be dropped. If there's no conditions the for loop gets skipped already.
| // No conditions yet means it will be set to ready during validation. | |
| if len(conditions) == 0 { | |
| return true | |
| } |
| // skip computing IR | ||
| return |
There was a problem hiding this comment.
Does this actually skip anything? I think this can be dropped.
| // skip computing IR | |
| return |
| // Per-gateway check: within each gateway, listeners on the same port must use compatible protocols. | ||
| // HTTPS and TLS are considered compatible with each other but not with HTTP or TCP. | ||
| for _, gateway := range gateways { | ||
| portListenerInfo := collectPortListeners(gateway.listeners) | ||
| markProtocolConflicts(portListenerInfo) | ||
| } | ||
|
|
||
| // In merge mode, also check for protocol conflicts across all gateways that share | ||
| // the same infra port, since they share the same IR key. A Gateway with HTTP/80 | ||
| // and another with TCP/80 would each pass the per-gateway check above (single | ||
| // listener per gateway), but must still be flagged as conflicted in merge mode. | ||
| if t.MergeGateways { | ||
| var allListeners []*ListenerContext | ||
| for _, gateway := range gateways { | ||
| allListeners = append(allListeners, gateway.listeners...) | ||
| } | ||
| portListenerInfo := collectPortListeners(allListeners) | ||
| markProtocolConflicts(portListenerInfo) | ||
| } |
There was a problem hiding this comment.
In merged gateways mode this would loop over all gateways twice.
| // Per-gateway check: within each gateway, listeners on the same port must use compatible protocols. | |
| // HTTPS and TLS are considered compatible with each other but not with HTTP or TCP. | |
| for _, gateway := range gateways { | |
| portListenerInfo := collectPortListeners(gateway.listeners) | |
| markProtocolConflicts(portListenerInfo) | |
| } | |
| // In merge mode, also check for protocol conflicts across all gateways that share | |
| // the same infra port, since they share the same IR key. A Gateway with HTTP/80 | |
| // and another with TCP/80 would each pass the per-gateway check above (single | |
| // listener per gateway), but must still be flagged as conflicted in merge mode. | |
| if t.MergeGateways { | |
| var allListeners []*ListenerContext | |
| for _, gateway := range gateways { | |
| allListeners = append(allListeners, gateway.listeners...) | |
| } | |
| portListenerInfo := collectPortListeners(allListeners) | |
| markProtocolConflicts(portListenerInfo) | |
| } | |
| // Detect listeners that share a port but use incompatible protocols. | |
| // HTTPS and TLS may coexist; HTTP and TCP may not coexist with anything else on the same port. | |
| // | |
| // In merge mode, all gateways collapse to a single IR key, so listeners must be | |
| // checked across the full set. Otherwise, conflicts are scoped per gateway. | |
| if t.MergeGateways { | |
| var all []*ListenerContext | |
| for _, g := range gateways { | |
| all = append(all, g.listeners...) | |
| } | |
| markProtocolConflicts(collectPortListeners(all)) | |
| } else { | |
| for _, g := range gateways { | |
| markProtocolConflicts(collectPortListeners(g.listeners)) | |
| } | |
| } |
Signed-off-by: zirain <[email protected]>
3 participants
Replace #8361