-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
minikube update-context: Fix nil pointer dereference #8989
minikube update-context: Fix nil pointer dereference #8989
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: superbrothers The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
I am glad you are doing a PR to improve this, could you please add a subtest to our exiting UpdateContext integration test?
you could do a subtest, by passing a KUBECONFIG env to the command that points to a problamtic kubeconfig.
for example, you can add something like this
https://github.com/medyagh/minikube/blob/5c73732d767154d168604dac8a45d5e534978a33/test/integration/functional_test.go#L1010-L1014
cmd= exec.CommandContext(ctx, Target(), "-p", profile, "update-context", "--alsologtostderr", "-v=2")
cmd.Env =..... (add a KUBECONFIG env here...you can create a dummy temp file or add the probalmatic kubeconfig to the testdata folder.
rr, err := Run(t,cmd)
if err != nil {
t.Errorf("failed to run minikube update-context: args %q: %v", rr.Command(), err)
}
@medyagh OK, I'll add a subtest for Update Context, but we need to decide If there is no minikube cluster in the kubeconfig (including if no clusters), add cluster, user and context settings to the kubeconfig, and set minikube to the current-context. This is the current behavior, but according to func TestUpdateIP(t *testing.T) {
var tests = []struct {
description string
hostname string
port int
existing []byte
err bool
status bool
expCfg []byte
}{
{
description: "no minikube cluster",
hostname: "192.168.10.100",
port: 8080,
existing: kubeConfigWithoutHTTPS,
err: true,
expCfg: kubeConfigWithoutHTTPS,
}, |
3ecf887
to
eb01a2e
Compare
@medyagh I updated a subtest to our exiting UpdateContext integration test at eb01a2e.
|
I'll squash the commits after the changes are reviewed. |
eb01a2e
to
bbf96e7
Compare
/ok-to-test |
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.
LGTM pending integration tests
kvm2 Driver |
Some tests seem to be failing on items not related to this PR. |
I tested this locally and it works great. Thank you! |
Fixes #8988