-
Notifications
You must be signed in to change notification settings - Fork 692
[kubectl-plugin][Refactor] Standardize namespace resolution to respect kubeconfig context. #4367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| namespace, _, err := options.cmdFactory.ToRawKubeConfigLoader().Namespace() | ||
| if err != nil{ | ||
| return "", fmt.Errorf("failed to get current namespace: %w", err) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Config file namespace ignored due to always-set options.namespace
High Severity
The resolveNamespace() function can no longer use the namespace from a config file. Previously, cmd.Flags().GetString("namespace") returned an empty string when the user didn't pass --namespace, allowing the config file namespace to be used. Now, ToRawKubeConfigLoader().Namespace() always returns a value (at minimum "default"), so options.namespace is never empty. This causes the condition if options.namespace != "" to always be true, making the else if branch that uses rayClusterConfig.Namespace unreachable. Additionally, the error check at line 249 will now incorrectly fail when a config file specifies a namespace different from the kubeconfig default.
Additional Locations (2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, and I think you should add test to check this
Future-Outlier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @win5923 @lorriexingfang @AndySung320 @400Ping to review
|
Currently working on resolving the failing tests. |
0d9c447 to
f7a7f45
Compare
ab69d83 to
1d28033
Compare
win5923
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI keeps failing. Could you take a look?
https://github.com/ray-project/kuberay/actions/runs/21089666265/job/60659060831?pr=4367
|
I tried but the CI that is failing is Would appreciate help for debugging and passing this test. |
|
Hi @kash2104 ,
|
b8db8cb to
e904e1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
4b21de4 to
b1ff884
Compare
AndySung320
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the Go-build-and-test / Lint (pre-commit) check is still failing.
Could you take a look and fix the lint issues?
437850d to
16f91e9
Compare
|
@AndySung320 Have fixed the failing tests. |
Future-Outlier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also change completion.go?
|
If we change |
Why are these changes needed?
Currently kubectl-plugin failed to respect the kubeconfig context by hardcoding the namespace value to default.
This PR uses Factory's namespace resolution in order to use namespace configured in kubeconfig.
Related issue number
Closes #4318
Checks