api: split ClusterSettings into BackendSettings and ClusterSettings - #8899
api: split ClusterSettings into BackendSettings and ClusterSettings#8899muwaqar-cflt wants to merge 5 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d5671b0a1
ℹ️ 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".
|
Hi @muwaqar-cflt, the new API shape looks good to me. Thanks for the quick PR! A couple of questions:
cc @envoyproxy/gateway-maintainers , please chime in. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8899 +/- ##
=======================================
Coverage 81.32% 81.33%
=======================================
Files 264 264
Lines 40964 40964
=======================================
+ Hits 33315 33318 +3
+ Misses 7648 7645 -3
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
74b8411 to
5e69afc
Compare
5e69afc to
159b4e2
Compare
a08ab35 to
9084c06
Compare
|
LGTM, thanks! |
9084c06 to
e3a3351
Compare
|
LGTM. Deferring to @guydc I'm also OK with delaying this to EG API graduating to V1. |
|
Can we roll out this change gradually and give users some grace to upgrade, e.g. at least one version where both exist side-by-side? If the effort is too large, I'm fine with this. Let's still document it as a breaking change though, so that users that rely on go types programtically are aware... |
…ields Extract CDS-only fields (LoadBalancer, ProxyProtocol, TCPKeepalive, HealthCheck, CircuitBreaker, Timeout, Connection, DNS, HTTP2) into BackendClusterSettings. ClusterSettings embeds it inline and adds Retry. JSON field paths are unchanged (non-breaking). Closes envoyproxy#8898 Signed-off-by: Muhammad Waqar <waqar.hameed08@gmail.com>
Address reviewer feedback to use a shorter, clearer name. Signed-off-by: Muhammad Waqar <waqar.hameed08@gmail.com>
… BackendSettings Swap type names per reviewer feedback: the CDS-only struct becomes ClusterSettings and the wrapper (with Retry) becomes BackendSettings. Also rename clustersettings.go to backendsettings.go. Signed-off-by: Muhammad Waqar <waqar.hameed08@gmail.com>
…s after rebase Rebasing onto main picked up test cases that construct BackendTrafficPolicySpec with a direct ClusterSettings field, which this branch's split moved one level deeper under the new BackendSettings field. git's auto-merge didn't flag these as conflicts since the surrounding lines didn't textually overlap. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
…king Addresses guydc's review comment on envoyproxy#8899 by documenting the Go-type impact instead of maintaining both types side-by-side for a release. Signed-off-by: Muhammad Waqar <mwaqar@confluent.io>
45f2f80
e3a3351 to
45f2f80
Compare
|
@guydc Rebased the PR onto current I looked at doing a real side-by-side transition, but it's more than it looks like: since |
|
/retest |
Summary
Extract CDS-only fields from the existing
ClusterSettingsinto a dedicatedClusterSettingsstruct, and rename the wrapper (which addsRetry) toBackendSettings.BackendSettingsembedsClusterSettingsinline and addsRetry. JSON field paths are unchanged — this is a non-breaking refactor.Closes #8898
Fields in
ClusterSettings(CDS-only)loadBalancerproxyProtocoltcpKeepalivehealthCheckcircuitBreakertimeoutconnectiondnshttp2Fields in
BackendSettings(wrapper)ClusterSettingsinlineretry(RDS concern, not applicable to Backend targets)Test plan
go build ./...succeedsmake lintpassesmake formatpassesmake gen-checkpasses