api: make RateLimitRedisSettings.url a pointer#9538
Conversation
Follow-up to envoyproxy#9143. url became optional (mutually exclusive with urlRef) but stayed a non-pointer string, so an unset value could not be distinguished from an explicit empty string. Make it *string so nil means unset, per review feedback. Signed-off-by: Andrey Maltsev <maltsev.andrey@gmail.com>
✅ 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: c6dccbfaf4
ℹ️ 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".
| Type: egv1a1.RedisBackendType, | ||
| Redis: &egv1a1.RateLimitRedisSettings{ | ||
| URL: ":foo", | ||
| URL: new(":foo"), |
There was a problem hiding this comment.
When any of the modified Go test packages are compiled, new(":foo") is invalid because new takes a type, not a value, so these updated tests fail before running. The same URL: new("...") pattern appears throughout this commit; use a string pointer helper such as ptr.To(...) or assign the literal to a variable and take its address.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a false positive. Go 1.26 (this module is go 1.26.4) added the two-argument-style new(expr) builtin that allocates and initializes from a value, so new(":foo") is valid and returns *string. The sibling RateLimitRedisSettings tests already use this form (Optional: new(true) / new(false)) from #9143 and pass upstream CI. go test ./... on the modified packages compiles and passes (473 tests).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9538 +/- ##
==========================================
- Coverage 75.59% 75.55% -0.04%
==========================================
Files 252 252
Lines 41713 41713
==========================================
- Hits 31531 31515 -16
- Misses 8057 8067 +10
- Partials 2125 2131 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| hasURL := redis.URL != "" | ||
| hasURL := redis.URL != nil |
What type of PR is this?
api
What this PR does / why we need it:
Follow-up to #9143. In that PR
RateLimitRedisSettings.urlbecame optional (mutually exclusive with the newurlRef) but stayed a non-pointerstring, so an unset value could not be distinguished from an explicit empty string. This changes it to*stringas requested in review, sonilmeans unset and thehas(self.url)CEL rule reflects field presence correctly.The JSON serialization is unchanged (
url,omitempty), so this is backward-compatible for the (unreleased) field. Go config validation and the rate-limit Deployment rendering are updated to deref the pointer; generated deepcopy and the API reference are regenerated.Which issue(s) this PR fixes:
Follow-up to #9143 (review comment from @arkodg).
Release Notes: No