Conversation
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cmd/server/start.go (1)
217-221:watch-cachememory-saving comment appears misleading for this codebase.Line 217 says this reduces memory, but
pkg/apiserver/apiserver.go(Lines 83-150) wires custom file-based storage withWatchDispatcher, not the generic cacher path. This flag likely has little/no effect here; suggest updating/removing this comment (or the flag write) to avoid operational confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/server/start.go` around lines 217 - 221, The comment claiming "disable watch cache to reduce memory usage" is misleading because the apiserver uses a custom file-based storage and WatchDispatcher (see pkg/apiserver/apiserver.go) so setting flags.Set("watch-cache","false") has little or no effect; either remove the comment and the flags.Set("watch-cache", "false") call in start.go, or replace the comment with a clear note that the watch-cache flag is ineffective here due to the custom WatchDispatcher wiring, keeping the code only if backward compatibility is desired; update the logger usage (logger.L().Warning) accordingly if you remove the flag so there is no misleading warning about failing to set an unused behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/server/start.go`:
- Around line 211-227: The flag enforcement for disabling ServerSideApply,
watch-cache, and enable-swagger-ui is currently done via flags.Set during
command construction (using flags.Set) so users can override them and failures
only warn; move these checks into the existing PersistentPreRunE hook so they
run after CLI parsing, call flags.Set for "feature-gates"
("ServerSideApply=false"), "watch-cache" ("false") and "enable-swagger-ui"
("false") there, and on any error return a non-nil error from PersistentPreRunE
(do not use logger.L().Warning) to fail closed; finally remove the duplicate
flags.Set block from the earlier command setup to avoid redundant pre-parse
defaults.
---
Nitpick comments:
In `@pkg/cmd/server/start.go`:
- Around line 217-221: The comment claiming "disable watch cache to reduce
memory usage" is misleading because the apiserver uses a custom file-based
storage and WatchDispatcher (see pkg/apiserver/apiserver.go) so setting
flags.Set("watch-cache","false") has little or no effect; either remove the
comment and the flags.Set("watch-cache", "false") call in start.go, or replace
the comment with a clear note that the watch-cache flag is ineffective here due
to the custom WatchDispatcher wiring, keeping the code only if backward
compatibility is desired; update the logger usage (logger.L().Warning)
accordingly if you remove the flag so there is no misleading warning about
failing to set an unused behavior.
|
Summary:
|
1 similar comment
|
Summary:
|
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
matthyx
force-pushed
the
server-side-apply
branch
from
18c2220 to
677a774
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/cmd/server/start.go (1)
136-138:⚠️ Potential issue | 🟠 MajorDo not bypass
ServerSideApply=falseenforcement whenskipDefaultComponentGlobalsRegistrySetis true.Line 136 returns before Line 143 runs, so this path skips the forced feature-gate entirely. If this flag is meant to be always enforced, the early return should only skip registry setup, not the enforcement block.
Suggested fix
PersistentPreRunE: func(cmd *cobra.Command, _ []string) error { - if skipDefaultComponentGlobalsRegistrySet { - return nil - } - if err := defaults.ComponentGlobalsRegistry.Set(); err != nil { - return err - } + if !skipDefaultComponentGlobalsRegistrySet { + if err := defaults.ComponentGlobalsRegistry.Set(); err != nil { + return err + } + } flags := cmd.Flags() if err := flags.Set("feature-gates", "ServerSideApply=false"); err != nil { return err } return nil },Also applies to: 143-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/server/start.go` around lines 136 - 138, The early return when skipDefaultComponentGlobalsRegistrySet is true currently prevents the ServerSideApply=false enforcement from running; change the control flow in start.go so the return only skips the registry setup and not the enforcement block: remove or move the return that uses skipDefaultComponentGlobalsRegistrySet so that the feature-gate enforcement (the ServerSideApply=false check around the code at/near lines referencing ServerSideApply) always executes, while still bypassing calls that set up the default component globals registry; reference the skipDefaultComponentGlobalsRegistrySet variable and the enforcement code around ServerSideApply to ensure enforcement always runs even when registry setup is skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/server/start.go`:
- Around line 142-145: The code currently replaces the entire "feature-gates"
flag via flags.Set("feature-gates", "ServerSideApply=false"); change this to
read the existing feature-gates value, parse/merge in the entry
ServerSideApply=false (overriding any existing ServerSideApply setting), and
write back the combined comma-separated value using flags.Set; update the logic
around flags.Set/flags.Get (the same flags variable and the "feature-gates" key)
so user-provided gates are preserved while ensuring ServerSideApply is set to
false.
---
Duplicate comments:
In `@pkg/cmd/server/start.go`:
- Around line 136-138: The early return when
skipDefaultComponentGlobalsRegistrySet is true currently prevents the
ServerSideApply=false enforcement from running; change the control flow in
start.go so the return only skips the registry setup and not the enforcement
block: remove or move the return that uses
skipDefaultComponentGlobalsRegistrySet so that the feature-gate enforcement (the
ServerSideApply=false check around the code at/near lines referencing
ServerSideApply) always executes, while still bypassing calls that set up the
default component globals registry; reference the
skipDefaultComponentGlobalsRegistrySet variable and the enforcement code around
ServerSideApply to ensure enforcement always runs even when registry setup is
skipped.
| flags := cmd.Flags() | ||
| if err := flags.Set("feature-gates", "ServerSideApply=false"); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Kubernetes/Cobra CLI flag handling, when calling cmd.Flags().Set("feature-gates", "ServerSideApply=false"), does this replace the entire feature-gates value or merge with existing entries (e.g. --feature-gates=Foo=true)?
💡 Result:
In Kubernetes/Cobra CLI flag handling, calling cmd.Flags.Set("feature-gates", "ServerSideApply=false") replaces the entire feature-gates value rather than merging with existing entries. The --feature-gates flag uses a custom pflag.Value implementation from k8s.io/component-base/cli/flag, specifically ColonSeparatedMultimapStringString. Its Set(string) method is designed such that "The first call to Set will clear the map before adding entries; subsequent calls will simply append to the map." When cmd.Flags.Set(name, value) is called, it invokes flag.Value.Set(value) on the flag's Value. For this custom type, the first Set clears any existing map contents (e.g., from prior CLI flags like --feature-gates=Foo=true or defaults) and parses the new string into the map. Thus, it fully replaces the value. This behavior enables overriding defaults or prior settings with a single new comma-separated list, without preserving previous entries.
Citations:
- 1: https://pkg.go.dev/k8s.io/component-base/cli/flag
- 2: https://github.com/kubernetes/component-base/blob/master/featuregate/feature_gate.go
🏁 Script executed:
# Examine the code context around lines 142-145 in pkg/cmd/server/start.go
head -160 pkg/cmd/server/start.go | tail -40 | cat -nRepository: kubescape/storage
Length of output: 1898
🏁 Script executed:
# Search for any feature-gates handling or merging logic in the codebase
rg "feature-gates|FeatureGates" --type go -A 3 -B 3 | head -100Repository: kubescape/storage
Length of output: 385
🏁 Script executed:
# Check where feature-gates flag is defined/registered
rg "feature-gates|featureGates|FeatureGates" --type go | head -50Repository: kubescape/storage
Length of output: 163
Merge feature-gates instead of replacing the entire value.
Line 143 calls flags.Set("feature-gates", "ServerSideApply=false"), which replaces the entire feature-gates value and drops any entries passed by users (e.g., --feature-gates=Foo=true). Retrieve the current value, merge in ServerSideApply=false, and set the combined result to preserve user-provided gates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cmd/server/start.go` around lines 142 - 145, The code currently replaces
the entire "feature-gates" flag via flags.Set("feature-gates",
"ServerSideApply=false"); change this to read the existing feature-gates value,
parse/merge in the entry ServerSideApply=false (overriding any existing
ServerSideApply setting), and write back the combined comma-separated value
using flags.Set; update the logic around flags.Set/flags.Get (the same flags
variable and the "feature-gates" key) so user-provided gates are preserved while
ensuring ServerSideApply is set to false.
|
Summary:
|
1 participant
Summary by CodeRabbit