oc login/project: Add support for custom context name #2187
Conversation
openshift-ci
bot
added
the
do-not-merge/hold
WalkthroughAdds a new --context flag and Context field to LoginOptions; implements mergeConfig to perform context-aware kubeconfig merging; updates SaveConfig to use mergeConfig; adds tests for merge behavior; separately adjusts project context-update logic to preserve custom contexts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Out-of-scope changes
✨ Finishing touches
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tchap The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/cli/login/login.go`:
- Line 168: NewCmdLogin reads o.Context via kcmdutil.GetFlagString in the Run
flow but never registers a "--context" flag, causing a runtime panic; fix by
registering the flag in NewCmdLogin (alongside other flags) with
cmds.Flags().StringVar tied to o.Context and a helpful usage string (e.g., "The
name of the kubeconfig context to use or create") so kcmdutil.GetFlagString can
safely read it at runtime.
In `@pkg/cli/login/loginoptions.go`:
- Around line 597-632: The mergeConfig method in LoginOptions doesn't update
ret.CurrentContext when o.Context (the --context flag) is provided, so the
active context remains unchanged; modify mergeConfig (in the LoginOptions type)
to set ret.CurrentContext = o.Context after you assign ret.Contexts[o.Context] =
&newContext (i.e., when o.Context != ""), mirroring the behavior of
cliconfig.MergeConfig so the specified context becomes the active
CurrentContext.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/cli/login/login.gopkg/cli/login/loginoptions.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cli/login/loginoptions.gopkg/cli/login/login.go
🧬 Code graph analysis (1)
pkg/cli/login/loginoptions.go (1)
pkg/helpers/kubeconfig/smart_merge.go (1)
MergeConfig(115-153)
🔇 Additional comments (3)
pkg/cli/login/login.go (1)
47-49: LGTM!The example clearly demonstrates the new
--contextflag usage.pkg/cli/login/loginoptions.go (2)
78-78: LGTM!Field addition is straightforward and appropriately placed.
574-577: LGTM!Clean refactoring to delegate merge logic to the new method.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
tchap
force-pushed
the
oc-login-keep-ctx-name-flag
branch
from
54475e2 to
ea89ae0
Compare
tchap
force-pushed
the
oc-login-keep-ctx-name-flag
branch
from
ea89ae0 to
2090bdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cli/login/loginoptions.go`:
- Around line 604-623: When reusing an existing context (existingContext) you
update additionalConfig.Clusters/AuthInfos but you never update the Context
object to point at those reused names, leaving Context.Cluster and
Context.AuthInfo pointing to the original created names; modify the loop that
sets additionalConfig.Contexts so that for the Context object stored under key
o.Context you also set ctx.Cluster = existingContext.Cluster and ctx.AuthInfo =
existingContext.AuthInfo (i.e. update the Context value from
additionalConfig.Contexts before assigning it back), and keep the existing logic
that remaps additionalConfig.Clusters and additionalConfig.AuthInfos to the
existingContext names so the context references match the renamed cluster/auth
entries.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
pkg/cli/login/login.gopkg/cli/login/loginoptions.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/cli/login/login.gopkg/cli/login/loginoptions.go
🧬 Code graph analysis (1)
pkg/cli/login/loginoptions.go (1)
pkg/helpers/kubeconfig/smart_merge.go (1)
MergeConfig(115-153)
🔇 Additional comments (4)
pkg/cli/login/loginoptions.go (2)
78-78: Context option addition looks good.
574-575: Using a dedicated merge helper is a clean upgrade.pkg/cli/login/login.go (2)
47-49: Example addition is clear and helpful.
168-168: Context flag wiring is correct.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
tchap
force-pushed
the
oc-login-keep-ctx-name-flag
branch
from
2090bdc to
2855dc9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/cli/login/loginoptions.go`:
- Around line 612-614: The code currently calls panic(errors.New("no context
found in additionalConfig")) when newContext is nil; replace this panic with
returning an error (e.g., return nil, fmt.Errorf("no context found in
additionalConfig") or simply return fmt.Errorf(...)) and propagate that error up
the caller chain by updating the enclosing function's signature to return an
error if necessary; locate the check around the newContext variable in
loginoptions.go and ensure callers of that function handle and surface the
returned error to the CLI user instead of allowing a panic.
The flag can be used to tell oc what context name to use when updating kubeconfig. If a matching context already exists, it is replaced altogether, including the linked cluster and authInfo. This means that the configured cluster and authInfo names are also reused.
tchap
force-pushed
the
oc-login-keep-ctx-name-flag
branch
from
2855dc9 to
f10a819
Compare
|
Added some tests. /unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@tchap I did some deep investigations today to understand why this hasn't been implemented yet. There was this openshift/origin#16161 PR which tried to adopt the similar approach with yours and it was rejected by openshift/origin#16161 (comment). The real reason is some commands in oc (i.e. |
When --context or the current context contain a custom context name, reuse that context if it matches the current --server value instead of always expecting the context match the generated context name. The change is entirely backwards compatible. When the current context matches the generated context name, the old functionality is retained.
tchap
changed the title
|
/retest |
1 similar comment
|
/retest |
|
@tchap: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Seems like this needs a minor test alignment in checking |
2 participants
The changes are explained below. We need to change both at once to make custom context names work.
oc login: Implement--contextflagThe flag can be used to tell
ocwhat context name to use when updatingkubeconfig. If a matching context already exists, it is replaced altogether, including the linkedclusterandauthInfo. This means that the configured clusterandauthInfo` names are also reused.oc project: Add support for custom context namesWhen
--contextor the current context contain a custom context name, reuse that context if it matches the current--servervalue instead of always expecting the context match the generated context name.The change is entirely backwards compatible. When the current context matches the generated context name, the old functionality is retained.
Fixes #283