Skip to content

Conversation

@jlewi
Copy link
Contributor

@jlewi jlewi commented Dec 27, 2017

  • setup_cluster needs to push the cluster name so that it is available to
    the teardown step before we try to setup the cluster so that the name
    is available even if setup_cluster fails.

  • setup_cluster also needs to handle the case where setup_cluster might
    already have been attempted; in which case we should reuse that cluster.

  • Fix E2E tests leaking GKE clusters #80


This change is Reviewable

* setup_cluster needs to push the cluster name so that it is available to
  the teardown step before we try to setup the cluster so that the name
  is available even if setup_cluster fails.

* setup_cluster also needs to handle the case where setup_cluster might
  already have been attempted; in which case we should reuse that cluster.
@jlewi
Copy link
Contributor Author

jlewi commented Dec 27, 2017

/cc @zjj2wry @jimexist @gaocegege @ScorpioCPH Can one of you review this please? (I think most of the folks in the US are out this week).

@coveralls
Copy link

coveralls commented Dec 27, 2017

Coverage Status

Coverage remained the same at 37.782% when pulling 5bb5295 on jlewi:cluster_leaks into e98b10c on tensorflow:master.

@ScorpioCPH
Copy link
Member

ScorpioCPH commented Dec 27, 2017

Sure, add related issue: #80

Copy link
Member

@ScorpioCPH ScorpioCPH left a comment

Choose a reason for hiding this comment

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

LGTM, sorry i don't have GKE env to run this script, so just review the code logic :)

@jlewi
Copy link
Contributor Author

jlewi commented Dec 27, 2017

Thanks.

@jlewi jlewi merged commit 420f9aa into kubeflow:master Dec 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants