feat(cli): add velero client config set namespace-mode=auto - #10127
feat(cli): add velero client config set namespace-mode=auto#10127lubronzhan wants to merge 5 commits into
Conversation
👷 Deploy request for velero pending review.Visit the deploys page to approve it
|
|
@kaovilai Since you proposed this approach (set-context-as-velero-namespace) on the issue, would appreciate your review here. Thanks! |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ce command Saves the namespace of the current (or a specified) kubeconfig context into the Velero client config file, so operational commands default to it without requiring --namespace on every invocation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: lubronzhan <lubron.zhan@broadcom.com>
- changelogs/unreleased must be named <pr-number>-<username>; rename from the 0000 placeholder to 10127 to satisfy hack/changelog-check.sh. - Extract the command's logic into setContextAsVeleroNamespace so it's testable without triggering os.Exit via cmd.CheckError, and add unit tests covering: namespace read from context, context with no explicit namespace, overwriting an existing config value, and invalid kubeconfig path. Addresses 0% codecov patch coverage on the PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: lubronzhan <lubron.zhan@broadcom.com>
30c710f to
95b7a84
Compare
kaovilai
left a comment
There was a problem hiding this comment.
Adds velero client set-context-as-velero-namespace to persist the current kubeconfig context's namespace into the Velero client config — a lower-risk alternative to changing default namespace resolution. has-unit-tests, CI green (61 checks), no outstanding review threads.
Note
Responses generated with Claude
|
Thanks for this easy kill. I think this is a cool PR to slightly improve usability. |
Thanks @kaovilai is it ok to merge this PR now? |
|
Our repository is set up so that two reviewers are required, so we will need another reviewer. Please wait. |
shubham-pampattiwar
left a comment
There was a problem hiding this comment.
Thanks for picking this up. The code is clean and well-tested. I have a few concerns about the approach before we merge.
CLI hierarchy: This command sits at velero client set-context-as-velero-namespace, but the existing way to set the namespace is velero client config set namespace=.... Adding a sibling command to client that does a subset of what config already handles breaks the CLI structure. This should live under velero client config if we add it.
Command naming: set-context-as-velero-namespace is hard to parse. It reads like "set the context as the velero namespace" when it actually means "read the namespace from my kubeconfig context and save it as velero's default." Something like velero client config set-from-context would be shorter and clearer.
Does this close #3194?: The original issue asks for the Velero CLI to automatically use the active namespace from the current kubeconfig context. This PR still requires the user to manually run a command every time they switch contexts. It's a convenience wrapper around the existing velero client config set namespace=... -- useful, but it doesn't address the core ask. I don't think we should close #3194 with this.
Value vs surface area: Today a user can achieve this with:
velero client config set namespace=$(kubectl config view --minify -o 'jsonpath={..namespace}')
Adding a dedicated command for this means more CLI surface to maintain and document. The tradeoff might be worth it if we get the placement and naming right, but as-is I'd want to see the command restructured before merging.
That's correct, the original ask propose a breaking change on the default behavior not sure we really wants that that's why I pick this approach. Let me update the description. |
Shubham suggested nesting the new command under `config` for hierarchy consistency, and renaming it since the original set-context-as-velero-namespace name was long and ambiguous. Moves it to `velero client config set-namespace-from-context`, matching the existing config get/set subcommands and my follow-up naming suggestion on the review thread. AI-Tool-Used: Claude Code AI-Tool-Use-Level: Category 3 (Low) AI-Code-Category: Category 1 (Production) Signed-off-by: lubronzhan <lubron.zhan@broadcom.com>
bf8d727 to
7ea7304
Compare
|
Hi @shubham-pampattiwar Please take a look see if you are ok with the new change. Thank you |
|
on original ask.. yeah do you think original ask is a more valid usecase? I thought its more convenient for velero to remember ns.. but if you are the type that bounce between two velero instances.. perhaps velero should defer to current ns as original issue ask. In that case, perhaps the config should allow for another option, auto namespace mode? |
@kaovilai MM make sense. And current command could be easily replaced by config set --namespace ? Not a big difference. So maybe a command to allow use to enable this auto context switch? Like Then do you think |
|
if we have |
kaovilai noted on velero-io#10127 that a one-shot command to snapshot the kubecontext namespace becomes redundant once a config toggle can resolve it dynamically, and isn't much simpler than the existing `config set namespace=...` alternative. Drop the dedicated set-namespace-from-context subcommand and instead teach the client Factory to resolve the operational namespace from the current kubeconfig context on every invocation when `namespace-mode=auto` is set via the existing generic `config set` command. Explicit --namespace flags and VELERO_NAMESPACE still take precedence, so the new mode only changes behavior when neither is set. AI-Tool-Used: Claude Code AI-Tool-Use-Level: Category 2 (Medium) AI-Code-Category: Category 1 (Production) Signed-off-by: lubronzhan <lubron.zhan@broadcom.com>
|
Good point @kaovilai, thanks. Replaced the one-shot |
kaovilai
left a comment
There was a problem hiding this comment.
Document how to disable auto mode. Since the generic setter deletes a key using an empty value, users apparently need velero client config set namespace-mode=. Adding that command to namespace.md would make the feature reversible and discoverable.
Add fallback/error-path coverage. Namespace() silently falls back to the stored/default namespace when kubeconfig namespace resolution fails. Even if this behavior is intentional because the interface cannot return an error, a test should lock it down and the docs should mention the fallback.
Use t.Setenv in new tests. This provides automatic cleanup and avoids environment leakage if a test exits early.
Resolve feedback from PR velero-io#10127 review 4966054980: - Document how to disable namespace-mode=auto (namespace-mode=) and note the fallback to the static namespace, in namespace.md. - Add a factory test covering the fallback to the stored/default namespace when kubeconfig namespace resolution fails. - Switch the VELERO_NAMESPACE override test to t.Setenv, wrapped in a subtest so its cleanup runs before later tests execute. AI-Tool-Used: Claude Code AI-Tool-Use-Level: Category 2 (Medium) AI-Code-Category: Category 2 (Non-Production)
Resolve feedback from PR velero-io#10127 review 4966054980: - Document how to disable namespace-mode=auto (namespace-mode=) and note the fallback to the static namespace, in namespace.md. - Add a factory test covering the fallback to the stored/default namespace when kubeconfig namespace resolution fails. - Switch the VELERO_NAMESPACE override test to t.Setenv, wrapped in a subtest so its cleanup runs before later tests execute. AI-Tool-Used: Claude Code AI-Tool-Use-Level: Category 2 (Medium) AI-Code-Category: Category 2 (Non-Production) Signed-off-by: lubronzhan <lubron.zhan@broadcom.com>
33d8485 to
6e76b92
Compare
Cool, done |
Summary
namespace-mode=autoas avelero client config setvalue. When set, Velero operational commands resolve their default namespace from the current kubeconfig context (or--kubecontext/--kubeconfigif specified) on every invocation, instead of a static stored value.--namespaceon every invocation.Test plan
go build ./...succeedsgo vet/gofmtclean on new/changed filesgo test ./pkg/client/... ./pkg/cmd/cli/client/...passes--namespaceflag overriding auto mode, andVELERO_NAMESPACEoverriding auto modeAI-Tool-Used: Claude Code
AI-Tool-Use-Level: Category 2 (Medium)
AI-Code-Category: Category 1 (Production)