Skip to content
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

add unset flag #187

Merged
merged 7 commits into from Feb 4, 2020
Merged

add unset flag #187

merged 7 commits into from Feb 4, 2020

Conversation

@rsalmond
Copy link
Contributor

rsalmond commented Dec 12, 2019

Fixes #185

@googlebot googlebot added the cla: yes label Dec 12, 2019
@ahmetb

This comment has been minimized.

Copy link
Owner

ahmetb commented Dec 12, 2019

Thanks, looks fairly unharmful. But

  1. I'm not entirely convinced if the linked proposal will have broad usage. Let's wait on that. We have to live with this flag for the rest of our life.
  2. We'll need integration tests implemented. :)
rsalmond added 3 commits Dec 31, 2019
@@ -0,0 +1,24 @@
# config with two contexts and a selected default

This comment has been minimized.

Copy link
@ahmetb

ahmetb Dec 31, 2019

Owner

Do we need a new file?
You can use another 2-context file, and set a context in the test (if that’s the only difference).

This comment has been minimized.

Copy link
@rsalmond

rsalmond Jan 2, 2020

Author Contributor

Yeah good question, I didn't want to make existing / future tests more complicated by overloading fixtures but happy to merge into config2 if you are.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jan 2, 2020

Owner

What I meant was that you can use an existing testdata file, and first set a context in test code, and then unset it. This way you wouldn't be modifying the existing testdata file.

This comment has been minimized.

Copy link
@rsalmond

rsalmond Jan 2, 2020

Author Contributor

Ah that makes more sense!

kubectx Outdated
@@ -185,6 +186,11 @@ delete_context() {
$KUBECTL config delete-context "${ctx}"
}

unset_context() {
echo "Unsetting default context." >&2

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jan 2, 2020

Owner

default -> current

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jan 2, 2020

Owner

I think this message is not really necessary, as kubectl will always print Property "current-context" unset. even though the file is empty.

$ KUBECONFIG=$(mktemp) kubectl config unset current-context
Property "current-context" unset.

So the "progress" message is not necessary.

This comment has been minimized.

Copy link
@rsalmond

rsalmond Jan 2, 2020

Author Contributor

Just going for consistency. Eg.

kubectx -d gke_dl-rapid_us-central1-a_rapid-dev
Deleting context "gke_dl-rapid_us-central1-a_rapid-dev"...
deleted context gke_dl-rapid_us-central1-a_rapid-dev from /Users/rsa/.kube/config
run ${COMMAND} -c
echo "$output"
[ "$status" -eq 1 ]
[[ "$output" = "error: current-context is not set" ]]

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jan 2, 2020

Owner

this message is coming from kubectl. we shouldn't test it as we have no control over it.

maybe let's just do this:

[ "$status" -ne 0 ] and test that there must be a failure.

tbh you can just omit this test in this patch. it's not about -u.

run ${COMMAND} -c
echo "$output"
[ "$status" -eq 1 ]
[[ "$output" = "error: current-context is not set" ]]

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jan 2, 2020

Owner

ditto let's not check the error message equality here, it's coming from kubectl, and may change.

rsalmond added 2 commits Jan 2, 2020
@@ -14,7 +14,7 @@ contexts:
cluster: cluster1
user: user2
name: user2@cluster1
current-context: ""
current-context: "user1@cluster1"

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jan 2, 2020

Owner

Please no modifications to this file. It might impact other tests. You can set the value in the test.

@ahmetb ahmetb merged commit 3369d42 into ahmetb:master Feb 4, 2020
2 checks passed
2 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ahmetb

This comment has been minimized.

Copy link
Owner

ahmetb commented Feb 4, 2020

Sorry I forgot to merge this lol. :)
Thanks for your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.