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

Support using our E2E workflow to build a Docker image for releases. #403

Merged
merged 32 commits into from
Feb 27, 2018

Conversation

jlewi
Copy link
Contributor

@jlewi jlewi commented Feb 23, 2018

  • Update our E2E test to use ksonnet not helm to deploy

    • As part of this we have a slightl loss in test coverage because our
      helm test provides some verification that our python tests don't.
  • Create a releasing environment for workflows that has parameters set
    as needed to build using our release cluster.

Related to
#400 Use Argo for releases
#373 Improve our test harness


This change is Reviewable

* Update our E2E test to use ksonnet not helm to deploy
   * As part of this we have a slightl loss in test coverage because our
     helm test provides some verification that our python tests don't.

* Create a releasing environment for workflows that has parameters set
  as needed to build using our release cluster.
@jlewi jlewi changed the title Support using our E2E workflow to build a Docker image for releases. [wip] Support using our E2E workflow to build a Docker image for releases. Feb 23, 2018
@coveralls
Copy link

coveralls commented Feb 23, 2018

Coverage Status

Coverage remained the same at 31.868% when pulling 93c2234 on jlewi:releases into ab1cb4d on kubeflow:master.

@jlewi
Copy link
Contributor Author

jlewi commented Feb 24, 2018

/test all

@jlewi jlewi changed the title [wip] Support using our E2E workflow to build a Docker image for releases. Support using our E2E workflow to build a Docker image for releases. Feb 24, 2018
@lluunn
Copy link
Contributor

lluunn commented Feb 26, 2018

Reviewed 2 of 35 files at r1, 4 of 11 files at r2.
Review status: 6 of 37 files reviewed at latest revision, 2 unresolved discussions.


py/deploy.py, line 72 at r3 (raw file):

  util.run(["ks", "env", "add", env], cwd=app_dir)

  for k, v in params.iteritems():

params might be None right?
Should check it first.
Also component should not be None, better check it too.


py/deploy.py, line 145 at r3 (raw file):

    logging.info("Using GCP account %s", account)
    util.run(["kubectl", "create", "clusterrolebinding", "default-admin",
              "--clusterrole=cluster-admin", "--user=" + account])

Why does this have to be run every time?


Comments from Reviewable

@gaocegege
Copy link
Member

gaocegege commented Feb 27, 2018

There are some conflicts now, please resolve them. And it seems that the coveralls does not report failure now, we could close the issue #406

@jlewi
Copy link
Contributor Author

jlewi commented Feb 27, 2018

Review status: 3 of 37 files reviewed at latest revision, 2 unresolved discussions.


py/deploy.py, line 72 at r3 (raw file):

Previously, lluunn (Lun-Kai Hsu) wrote…

params might be None right?
Should check it first.
Also component should not be None, better check it too.

params should never be None; it could be an empty dictionary.

Added a check for component although it too should always be uspplied.


py/deploy.py, line 145 at r3 (raw file):

Previously, lluunn (Lun-Kai Hsu) wrote…

Why does this have to be run every time?

It needs to be run once per cluster. The TFJob operator test creates a new cluster for each test run. We don't reuse clusters.


Comments from Reviewable

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

LGTM

Do we already use ksonnet instead of helm in our e2e test after the PR?

@jlewi
Copy link
Contributor Author

jlewi commented Feb 27, 2018

@gaocegege Yes we use ksonnet not helm after this PR to deploy the operator.

@jlewi jlewi merged commit c54cda9 into kubeflow:master Feb 27, 2018
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.

5 participants