oc login/project: Add support for custom context name#2187
oc login/project: Add support for custom context name#2187tchap wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/hold
WalkthroughAdds a new --context flag and LoginOptions.Context; implements context-aware kubeconfig merging via mergeConfig; updates SaveConfig to use mergeConfig; adds TestMergeConfig; adjusts project context update logic to reuse existing custom contexts when server matches. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Out-of-scope changes
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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.
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. |
tchap
changed the title
|
/retest |
1 similar comment
|
/retest |
|
Seems like this needs a minor test alignment in checking |
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.
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
force-pushed
the
oc-login-keep-ctx-name-flag
branch
from
c141ce0 to
5d1a333
Compare
|
@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. |
| contextUpdated := false | ||
| if currentContext != nil { | ||
| defaultContextName := cliconfig.GetContextNickname(currentContext.Namespace, currentContext.Cluster, currentContext.AuthInfo) | ||
| if config.CurrentContext != defaultContextName { |
There was a problem hiding this comment.
Logic in this PR makes sense to me. However, historically ProjectName always aligns with contextName (
Line 225 in 7e80dba
I understand the inconvenience filed in the issue. But it is more about annoyance not an actual bug.
Initially I was supportive of this work. But now I think I would prefer waiting from official RFE from customers to be driven by BU that support us to change the flow. This PR is already mergeable for me.
There was a problem hiding this comment.
Alright, didn't know the context name is so important to match. Thanks for the clarification.
There was a problem hiding this comment.
Me neither until seeing openshift/origin#16161 (comment). But it is good to have your PR in case we need it in the future.
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