-
Notifications
You must be signed in to change notification settings - Fork 1.3k
add unset flag #187
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
Conversation
Thanks, looks fairly unharmful. But
|
test/testdata/config3
Outdated
@@ -0,0 +1,24 @@ | |||
# config with two contexts and a selected default |
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.
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).
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.
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.
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.
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.
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.
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 |
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.
default -> current
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 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.
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.
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
test/kubectx.bats
Outdated
run ${COMMAND} -c | ||
echo "$output" | ||
[ "$status" -eq 1 ] | ||
[[ "$output" = "error: current-context is not set" ]] |
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.
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
.
test/kubectx.bats
Outdated
run ${COMMAND} -c | ||
echo "$output" | ||
[ "$status" -eq 1 ] | ||
[[ "$output" = "error: current-context is not set" ]] |
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.
ditto let's not check the error message equality here, it's coming from kubectl, and may change.
test/testdata/config2
Outdated
@@ -14,7 +14,7 @@ contexts: | |||
cluster: cluster1 | |||
user: user2 | |||
name: user2@cluster1 | |||
current-context: "" | |||
current-context: "user1@cluster1" |
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.
Please no modifications to this file. It might impact other tests. You can set the value in the test.
Sorry I forgot to merge this lol. :) |
Is this gonna be in any release soon? |
Released v0.8.0. |
Fixes #185