Unset the current-context after minikube stop #4177

hpandeycodeit · 2019-04-30T07:09:10Z

This PR implements Feature Request #4154

k8s-ci-robot · 2019-04-30T07:09:17Z

Hi @hpandeycodeit. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions 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/test-infra repository.

k8s-ci-robot · 2019-04-30T07:09:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hpandeycodeit
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jimmidyson

If they are not already assigned, you can assign the PR to them by writing /assign @jimmidyson in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

minikube-bot · 2019-04-30T07:10:03Z

Can one of the admins verify this patch?

tstromberg · 2019-04-30T17:15:52Z

@minikube-bot OK to test

tstromberg

This looks great, and will be a really nice improvement for users, particularly those who show their kubectl context in their shells. This PR looks like it does exactly what it should be doing.

To guard against breaking this in the future, do you mind adding a few lines to start_stop_delete_test.go to verify the behavior? For example, before you call stop, assert that there is a context, and that it goes away after stop. Something like:

`k = util.NewKubeCtlRunner()
got := k.RunCommand("config", "current-context")
if got != "minikube" { fail }


and after stop:

got := k.RunCommand("config", "current-context")
if got != "" { fail }


Thank you again for your contribution!

tstromberg · 2019-04-30T17:15:18Z

pkg/util/kubeconfig.go

@@ -115,6 +115,7 @@ func PopulateKubeConfig(cfg *KubeConfigSetup, kubecfg *api.Config) error {
 	// Only set current context to minikube if the user has not used the keepContext flag
 	if !cfg.KeepContext {
 		kubecfg.CurrentContext = cfg.ClusterName
+		//kubecfg.CurrentContext = ""


tstromberg · 2019-04-30T17:22:51Z

pkg/util/kubeconfig.go

@@ -312,3 +313,16 @@ func GetPortFromKubeConfig(filename, machineName string) (int, error) {
 	port, err := strconv.Atoi(kport)
 	return port, err
 }
+
+//UnsetCurrentContext unsets the current-context from minikube to "" on minikube stop
+func UnsetCurrentContext(filename, machineName string)  error {


Please run gofmt -s -w on this file to reformat it to Go standards. Consider configuring your code editor to do this on save.

formatted this one.

hpandeycodeit · 2019-04-30T22:34:45Z

@tstromberg I have included the tests. Let me know if it looks okay. Thanks for the pointer :)

tstromberg · 2019-04-30T22:42:23Z

Looks great, but you'll need to do a tiny bit more work to plumb the runner through correctly:

test/integration/start_stop_delete_test.go:81:4: undefined: kubeRunner
test/integration/start_stop_delete_test.go:81:17: undefined: "k8s.io/minikube/test/integration/util".NewKubeCtlRunner
test/integration/start_stop_delete_test.go:82:4: undefined: currentContext
test/integration/start_stop_delete_test.go:82:21: undefined: kubeRunner
test/integration/start_stop_delete_test.go:83:7: undefined: currentContext
test/integration/start_stop_delete_test.go:92:4: undefined: currentContext
test/integration/start_stop_delete_test.go:92:21: undefined: kubeRunner
test/integration/start_stop_delete_test.go:93:7: undefined: currentContext

Also, please run gofmt -s -w on the files you changed (cmd/minikube/cmd/stop.go in particular) so that they are formatted with the recommended Go style. FYI, you can run the integration tests locally by using something like:

make test
env TEST_ARGS="-minikube-start-args=--vm-driver=kvm2 -test.run TestStartStop" make integration

hpandeycodeit · 2019-04-30T23:25:50Z

@tstromberg Fixed the gofmt errors. Integration tests passed as well. Hopefully it should be fine this time.

RA489 · 2019-05-02T06:24:14Z

@minikube-bot OK to test

tstromberg · 2019-05-02T16:14:36Z

@minikube-bot OK to test

tstromberg · 2019-05-02T18:15:24Z

Tests are failing:

--- FAIL: TestStartStop (812.27s)
    --- FAIL: TestStartStop/nocache_oldest (166.82s)
        start_stop_delete_test.go:87: current-context not set to minikube
    --- FAIL: TestStartStop/feature_gates_newest_cni (199.59s)
        start_stop_delete_test.go:87: current-context not set to minikube
    --- FAIL: TestStartStop/containerd_and_non_default_apiserver_port (224.69s)
        start_stop_delete_test.go:87: current-context not set to minikube
    --- FAIL: TestStartStop/crio_ignore_preflights (221.17s)
        start_stop_delete_test.go:87: current-context not set to minikube

tstromberg · 2019-05-02T18:16:48Z

It might help here if the test showed the error in got vs want syntax, such as:

got context %q, want context %q

hpandeycodeit · 2019-05-02T19:18:49Z

@tstromberg I have changed it. Not sure why the context is different. Not getting this error in my local though.

hpandeycodeit · 2019-05-03T21:01:14Z

@tstromberg thanks for approving the changes!

Above failing tests still showing a failure in current-context mismatch.

--- FAIL: TestStartStop (826.16s) --- FAIL: TestStartStop/nocache_oldest (179.80s) start_stop_delete_test.go:87: got current-context - "minikube\n", want current-context "minikube" --- FAIL: TestStartStop/feature_gates_newest_cni (203.97s) start_stop_delete_test.go:87: got current-context - "minikube\n", want current-context "minikube" --- FAIL: TestStartStop/containerd_and_non_default_apiserver_port (222.13s) start_stop_delete_test.go:87: got current-context - "minikube\n", want current-context "minikube" --- FAIL: TestStartStop/crio_ignore_preflights (220.26s) start_stop_delete_test.go:87: got current-context - "minikube\n", want current-context "minikube" === RUN TestVersionUpgrade

not sure from where it's getting "minikube\n"

Any Idea?

tstromberg · 2019-05-03T21:01:44Z

--- FAIL: TestStartStop (947.48s)
    --- FAIL: TestStartStop/nocache_oldest (210.63s)
        start_stop_delete_test.go:87: got current-context - "minikube\n", want  current-context "minikube"
    --- FAIL: TestStartStop/feature_gates_newest_cni (223.77s)
        start_stop_delete_test.go:87: got current-context - "minikube\n", want  current-context "minikube"
    --- FAIL: TestStartStop/containerd_and_non_default_apiserver_port (252.40s)
        start_stop_delete_test.go:87: got current-context - "minikube\n", want  current-context "minikube"
    --- FAIL: TestStartStop/crio_ignore_preflights (260.68s)

You may want to call strings.TrimRight() before comparing the value.

tstromberg · 2019-05-06T19:00:16Z

Looks like it's getting better, but something is still not quite right:

--- FAIL: TestStartStop (857.56s)
    --- FAIL: TestStartStop/nocache_oldest (186.06s)
        start_stop_delete_test.go:100: Failed to unset the current-context "minikube\n"
    --- FAIL: TestStartStop/feature_gates_newest_cni (206.81s)
        start_stop_delete_test.go:100: Failed to unset the current-context "minikube\n"
    --- FAIL: TestStartStop/containerd_and_non_default_apiserver_port (238.16s)
        start_stop_delete_test.go:100: Failed to unset the current-context "minikube\n"
    --- FAIL: TestStartStop/crio_ignore_preflights (226.53s)
        start_stop_delete_test.go:100: Failed to unset the current-context "minikube\n"

Does running env TEST_ARGS="-test.run TestStartStop" make integration work for you?

hpandeycodeit · 2019-05-07T16:01:28Z

@tstromberg I am able to test this in my local and it's failing as well. So the issue here is, if I put this code right below the stop, since the minikube takes time to stop and the code still retrieves current-context as "minikube":

checkStop := func() error {
	r.RunCommand("stop", true)
	return r.CheckStatusNoFail(state.Stopped.String())
}

currentContext, err = kubectlRunner.RunCommand([]string{"config", "current-context"})
if strings.TrimRight(string(currentContext), "\n") != "" {
	t.Fatalf("Failed to unset the current-context %q", strings.TrimRight(string(currentContext), "\n"))
}

if err := util.Retry(t, checkStop, 5*time.Second, 6); err != nil {
	t.Fatalf("timed out while checking stopped status: %v", err)
}

However if I put this code after the code that checks stopped status, minikube is stopped then but there is no context and then it throws error something like this

util.go:295: Temporary Error: error running command [config current-context]: exit status 1. Stdout:
error: current-context is not set

So looks like I can't really compare the current-context with an empty string here ""

hpandeycodeit · 2019-05-13T17:34:37Z

@tstromberg let me know if this looks okay to you or any other change is needed to the test.

tstromberg · 2019-05-15T03:28:25Z

Looks good. Thank you!

hpandeycodeit · 2019-05-15T16:10:49Z

Thank you @tstromberg!

Unset the current-context on minikube stop

df0a0ad

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 30, 2019

k8s-ci-robot requested review from dlorenc and sharifelgamal April 30, 2019 07:09

tstromberg suggested changes Apr 30, 2019

View reviewed changes

Added tests to check current-context on start and stop of minikube

c8af727

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 30, 2019

Fixed gofmt errors

fb0127f

Fixed the build error and formatting

a4018bf

Fixed the test case

a533b56

Changed the error message style in test case.

fd7277b

tstromberg approved these changes May 2, 2019

View reviewed changes

minor code change: Added strings.TrimRight()

dd8d3cf

Added a param in the test case to find the current-context after stop

e23a784

Updated the test case

64f990c

hpandeycodeit mentioned this pull request May 9, 2019

REQUEST: New membership for @hpandeycodeit kubernetes/org#805

Closed

6 tasks

tstromberg merged commit c55b916 into kubernetes:master May 15, 2019

pdecat mentioned this pull request Jul 9, 2020

Start in context view if current-context is empty derailed/k9s#799

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unset the current-context after minikube stop #4177

Unset the current-context after minikube stop #4177

hpandeycodeit commented Apr 30, 2019

k8s-ci-robot commented Apr 30, 2019

k8s-ci-robot commented Apr 30, 2019

minikube-bot commented Apr 30, 2019

tstromberg commented Apr 30, 2019

tstromberg left a comment

tstromberg Apr 30, 2019

hpandeycodeit Apr 30, 2019

tstromberg Apr 30, 2019

hpandeycodeit Apr 30, 2019

hpandeycodeit commented Apr 30, 2019

tstromberg commented Apr 30, 2019 •

edited

Loading

hpandeycodeit commented Apr 30, 2019

RA489 commented May 2, 2019

tstromberg commented May 2, 2019

tstromberg commented May 2, 2019

tstromberg commented May 2, 2019

hpandeycodeit commented May 2, 2019

hpandeycodeit commented May 3, 2019 •

edited

Loading

tstromberg commented May 3, 2019

tstromberg commented May 6, 2019 •

edited

Loading

hpandeycodeit commented May 7, 2019

hpandeycodeit commented May 13, 2019

tstromberg commented May 15, 2019

hpandeycodeit commented May 15, 2019

Unset the current-context after minikube stop #4177

Unset the current-context after minikube stop #4177

Conversation

hpandeycodeit commented Apr 30, 2019

k8s-ci-robot commented Apr 30, 2019

k8s-ci-robot commented Apr 30, 2019

minikube-bot commented Apr 30, 2019

tstromberg commented Apr 30, 2019

tstromberg left a comment

Choose a reason for hiding this comment

tstromberg Apr 30, 2019

Choose a reason for hiding this comment

hpandeycodeit Apr 30, 2019

Choose a reason for hiding this comment

tstromberg Apr 30, 2019

Choose a reason for hiding this comment

hpandeycodeit Apr 30, 2019

Choose a reason for hiding this comment

hpandeycodeit commented Apr 30, 2019

tstromberg commented Apr 30, 2019 • edited Loading

hpandeycodeit commented Apr 30, 2019

RA489 commented May 2, 2019

tstromberg commented May 2, 2019

tstromberg commented May 2, 2019

tstromberg commented May 2, 2019

hpandeycodeit commented May 2, 2019

hpandeycodeit commented May 3, 2019 • edited Loading

tstromberg commented May 3, 2019

tstromberg commented May 6, 2019 • edited Loading

hpandeycodeit commented May 7, 2019

hpandeycodeit commented May 13, 2019

tstromberg commented May 15, 2019

hpandeycodeit commented May 15, 2019

tstromberg commented Apr 30, 2019 •

edited

Loading

hpandeycodeit commented May 3, 2019 •

edited

Loading

tstromberg commented May 6, 2019 •

edited

Loading