From 93c22340428047c196001452e1258c01afda565a Mon Sep 17 00:00:00 2001 From: Jeremy Lewi Date: Mon, 26 Feb 2018 19:14:51 -0800 Subject: [PATCH] Address some of Lunkai's comments. --- build/images/tf_operator/build_and_push.py | 87 +++++++++------------- py/deploy.py | 9 ++- 2 files changed, 45 insertions(+), 51 deletions(-) diff --git a/build/images/tf_operator/build_and_push.py b/build/images/tf_operator/build_and_push.py index 67f622f3fc..4f9f5c4225 100755 --- a/build/images/tf_operator/build_and_push.py +++ b/build/images/tf_operator/build_and_push.py @@ -16,17 +16,16 @@ # TODO(jlewi): build_and_push.py should be obsolete. We should be able to # use py/release.py - def GetGitHash(root_dir): # The image tag is based on the githash. - git_hash = subprocess.check_output( - ["git", "rev-parse", "--short", "HEAD"], cwd=root_dir).decode("utf-8") + git_hash = subprocess.check_output(["git", "rev-parse", "--short", "HEAD"], + cwd=root_dir).decode("utf-8") git_hash = git_hash.strip() - modified_files = subprocess.check_output( - ["git", "ls-files", "--modified"], cwd=root_dir) + modified_files = subprocess.check_output(["git", "ls-files", "--modified"], + cwd=root_dir) untracked_files = subprocess.check_output( - ["git", "ls-files", "--others", "--exclude-standard"], cwd=root_dir) + ["git", "ls-files", "--others", "--exclude-standard"], cwd=root_dir) if modified_files or untracked_files: diff = subprocess.check_output(["git", "diff"], cwd=root_dir) @@ -55,43 +54,34 @@ def run_and_output(command, cwd=None): def main(): # pylint: disable=too-many-locals, too-many-statements logging.getLogger().setLevel(logging.INFO) # pylint: disable=too-many-locals, too-many-statements parser = argparse.ArgumentParser( - description="Build docker image for TFJob CRD.") + description="Build docker image for TFJob CRD.") # TODO(jlewi) We should make registry required to avoid people accidentally # pushing to tf-on-k8s-dogfood by default. parser.add_argument( - "--registry", - default="gcr.io/tf-on-k8s-dogfood", - type=str, - help="The docker registry to use.") - - parser.add_argument( - "--project", - default="", - type=str, - help="Project to use with Google Container Builder when using GCB.") + "--registry", + default="gcr.io/tf-on-k8s-dogfood", + type=str, + help="The docker registry to use.") parser.add_argument( - "--output", - default="", - type=str, - help="Path to write a YAML file with build info.") + "--project", + default="", + type=str, + help="Project to use with Google Container Builder when using GCB.") parser.add_argument( - "--gcb", - dest="use_gcb", - action="store_true", - help="Use Google Container Builder to build the image.") - parser.add_argument( - "--no-gcb", - dest="use_gcb", - action="store_false", - help="Use Docker to build the image.") - parser.add_argument( - "--no-push", - dest="should_push", - action="store_false", - help="Do not push the image once build is finished.") + "--output", + default="", + type=str, + help="Path to write a YAML file with build info.") + + parser.add_argument("--gcb", dest="use_gcb", action="store_true", + help="Use Google Container Builder to build the image.") + parser.add_argument("--no-gcb", dest="use_gcb", action="store_false", + help="Use Docker to build the image.") + parser.add_argument("--no-push", dest="should_push", action="store_false", + help="Do not push the image once build is finished.") parser.set_defaults(use_gcb=False) @@ -114,9 +104,9 @@ def main(): # pylint: disable=too-many-locals, too-many-statements go_path = os.environ["GOPATH"] targets = [ - "github.com/kubeflow/tf-operator/cmd/tf-operator", - "github.com/kubeflow/tf-operator/test/e2e", - "github.com/kubeflow/tf-operator/dashboard/backend", + "github.com/kubeflow/tf-operator/cmd/tf-operator", + "github.com/kubeflow/tf-operator/test/e2e", + "github.com/kubeflow/tf-operator/dashboard/backend", ] for t in targets: subprocess.check_call(["go", "install", t]) @@ -129,10 +119,11 @@ def main(): # pylint: disable=too-many-locals, too-many-statements root_dir = os.path.abspath(os.path.join(images_dir, '..', '..')) # List of paths to copy relative to root. sources = [ - "build/images/tf_operator/Dockerfile", - os.path.join(go_path, "bin/tf-operator"), - os.path.join(go_path, "bin/e2e"), - os.path.join(go_path, "bin/backend"), "dashboard/frontend/build" + "build/images/tf_operator/Dockerfile", + os.path.join(go_path, "bin/tf-operator"), + os.path.join(go_path, "bin/e2e"), + os.path.join(go_path, "bin/backend"), + "dashboard/frontend/build" ] for s in sources: @@ -152,15 +143,12 @@ def main(): # pylint: disable=too-many-locals, too-many-statements latest_image = image_base + ":latest" if args.use_gcb: - run([ - "gcloud", "container", "builds", "submit", context_dir, "--tag=" + image, - "--project=" + args.project - ]) + run(["gcloud", "container", "builds", "submit", context_dir, + "--tag=" + image, "--project=" + args.project]) # Add the latest tag. - run([ - "gcloud", "container", "images", "add-tag", "--quiet", image, latest_image - ]) + run(["gcloud", "container", "images", "add-tag", "--quiet", image, + latest_image]) else: run(["docker", "build", "-t", image, context_dir]) @@ -181,6 +169,5 @@ def main(): # pylint: disable=too-many-locals, too-many-statements with open(args.output, mode='w') as hf: yaml.dump(output, hf) - if __name__ == "__main__": main() diff --git a/py/deploy.py b/py/deploy.py index 9e36497f1a..486ddf3310 100755 --- a/py/deploy.py +++ b/py/deploy.py @@ -52,11 +52,18 @@ def ks_deploy(app_dir, component, params, env=None, account=None): Args: app_dir: The ksonnet directory component: Name of the component to deployed - params: A dictionary of parameters to set. + params: A dictionary of parameters to set; can be empty but should not be + None. env: (Optional) The environment to use, if none is specified a new one is created. account: (Optional) The account to use. + + Raises: + ValueError: If input arguments aren't valid. """ + if not component: + raise ValueError("component can't be None.") + # TODO(jlewi): It might be better if the test creates the app and uses # the latest stable release of the ksonnet configs. That however will cause # problems when we make changes to the TFJob operator that require changes