Skip to content

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

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

Fixes #185

@ahmetb
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. :)

@@ -0,0 +1,24 @@
# config with two contexts and a selected default
Copy link
Owner

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).

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Contributor Author

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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default -> current

Copy link
Owner

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.

Copy link
Contributor Author

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

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

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.

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

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.

@@ -14,7 +14,7 @@ contexts:
cluster: cluster1
user: user2
name: user2@cluster1
current-context: ""
current-context: "user1@cluster1"
Copy link
Owner

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.

@ahmetb ahmetb merged commit 3369d42 into ahmetb:master Feb 4, 2020
@ahmetb
Copy link
Owner

ahmetb commented Feb 4, 2020

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

@jorgemoralespou
Copy link

Is this gonna be in any release soon?

@ahmetb
Copy link
Owner

ahmetb commented Feb 20, 2020

Released v0.8.0.

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

Successfully merging this pull request may close these issues.

Allow to unset the current-context (-u?)
4 participants