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.

Sign up for GitHub

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

Jump to bottom

Remove cluster from kubeconfig when deleting #226

Merged
merged 4 commits into from
Sep 28, 2018
Merged

Remove cluster from kubeconfig when deleting #226

merged 4 commits into from
Sep 28, 2018

Conversation

kschumy
Copy link

@kschumy kschumy commented Sep 23, 2018 •
edited
Loading

Description

If the kubeconfig contains an eksctl-created cluster, deleting the cluster will now remove the cluster from the kubeconfig.

Checklist

  • Code compiles correctly (i.e make build)
  • Added tests that cover your change (if possible)
  • All tests passing (i.e. make test)
  • Added/modified documentation as required (such as the README)
  • Added yourself to the humans.txt file

richardcase
richardcase reviewed Sep 24, 2018
View reviewed changes
Copy link
Contributor

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

Great work @kschumy, thanks!! My only comment would be, do you think it would be possible to get some tests around the deletion from the kubeconfig file?

I was never happy with the fact that i didn't add full deletion when i changed the code around the kubeconfig file. So this is great!

@kschumy
Copy link
Author

kschumy commented Sep 24, 2018

I'm glad you like it! I'll work on tests and get back to you. Thanks!

@richardcase
Copy link
Contributor

I'll work on tests and get back to you

Great, thanks @kschumy. The tests for the kubeconfig package use standard Go tests but for other packages we have started to use Gingko (for example, the ami package). Its easier to stick with the current test approach (i.e. standard go test) with this package but if you feel like going down the Ginkgo route then thats good as well.

Let me know if i can help.

@kschumy kschumy mentioned this pull request Sep 26, 2018
Closed
@kschumy
Copy link
Author

kschumy commented Sep 27, 2018

Sorry! I didn't see your comment until now. I created a test but did not use Ginkgo, but I'll do that in the future

richardcase
richardcase previously approved these changes Sep 27, 2018
View reviewed changes
Copy link
Contributor

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests around the new deletion code!

Ideally it would be good to have the 2 yaml files in a testdata subfolder. However, we could do this in the future when we refactor the tests to use Ginkgo and golden files (i just created
#230 for this).

@richardcase richardcase mentioned this pull request Sep 27, 2018
Closed
richardcase
richardcase suggested changes Sep 28, 2018
View reviewed changes
Copy link
Contributor

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

Just a couple of small changes now the test files have been moved.

Would you also be able to rebase on master? This will be good to go then, thanks again, great work.

pkg/utils/kubeconfig/kubeconfig_test.go Outdated
// All the information for cluster-one.us-west-2.eksctl.io is identical to one-cluster.yaml. If all
// the information for cluster-two.us-west-2.eksctl.io is deleted, the file should be identical to
// one-cluster.yaml and oneClusterAsBytes.
var twoClustersAsBytes, _ = ioutil.ReadFile("./two-clusters.yaml")

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

pkg/utils/kubeconfig/kubeconfig_test.go Outdated

// Cluster name is 'cluster-one.us-west-2.eksctl.io'.
// All the information is identical to cluster cluster-one.us-west-2.eksctl.io in two-clusters.yaml.
var oneClusterAsBytes, _ = ioutil.ReadFile("./one-cluster.yaml")

This comment was marked as abuse.

Sign in to view

Kirsten Schumy added 4 commits September 28, 2018 12:14
Removes cluster from kubeconfig when deleting
4264452 
If the kubeconfig contains an eksctl-created cluster, deleting the cluster will now remove the cluster from the kubeconfig.
Created kubeconfig yaml files for tests
326e8bd
Added MaybeDeleteConfig test and helper functions
e4b03ff 
Checks to see if MaybeDeleteConfig removes a cluster from the kubeconfig when deleting the cluster if the kubeconfig has the cluster's information and does not change the kubeconfig if it does not.

Also adds functions to supportthe test, such as managing the KUBECONFIG env variable, getting a clusterconfig example, and converting example kubeconfig yaml files into bytes.
Moved yamls to testdata and changed paths
dc17611
richardcase
richardcase approved these changes Sep 28, 2018
View reviewed changes
Copy link
Contributor

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for changing and rebasing. Feel free to merge when you get time.

@kschumy kschumy merged commit b3ee267 into eksctl-io:master Sep 28, 2018
@richardcase richardcase mentioned this pull request Oct 4, 2018
Closed
@errordeveloper
Copy link
Contributor

@kschumy awesome, thanks a lot for this PR! 👍

torredil pushed a commit to torredil/eksctl that referenced this pull request May 20, 2022
@k8s-ci-robot
Merge pull request eksctl-io#226 from mesosphere/dkoshkin/block-volum…
c716388 
…e-e2e

New block volume e2e tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardcase richardcase richardcase approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kschumy @richardcase @errordeveloper