Provide an error message in case GCP ZONE is not set and exit, also fix styles for kfctl.sh #1888

ashahba · 2018-10-30T06:47:57Z

This change is

…ix styles for kfctl.sh

ashahba · 2018-10-30T06:48:23Z

We should not enforce users to use us-east1-d which we only need for testing.
However during test runs we already provide --zone=us-east1-d, so we are ok to delete it I guess.

ashahba · 2018-10-30T06:48:33Z

/assign @jlewi

…ix styles for kfctl.sh

…hahba/fix-310

ashahba · 2018-10-30T16:35:52Z

/retest

…-310

ashahba · 2018-10-31T00:24:03Z

@jlewi looks like we never parsed --zone in kfctl.sh and we insisted on ZONE environment variable and forced user to use us-east1-d even if they run something like this:

./kfctl.sh init kfctl-2c94 --platform gcp --project intelai-mlt --zone us-west1-1 --s kipInitProject

and we failed.
The issue is fixed now and should fix the customer reported issue.

jlewi · 2018-10-31T17:10:26Z

scripts/kfctl.sh

@@ -137,6 +140,10 @@ if [ "${COMMAND}" == "init" ]; then
            shift
            PROJECT=$1
            ;;
+        --zone)


Can we have only one way to explicitly set zone? e.g. if we set it as a command line argument then we should not set it using an environment variable.

@jlewi Sure, but we are already doing same practice for --platform, PLATFORM, --project, PROJECT, --email and EMAIL so I thought that's kind of expected pattern here 🙂

Let me know what you think. But if we are taking one out in the favor of other I would choose to keep --zone over environment variable.

Let me know your thoughts.

I'm in favor of moving things to flags.

…-310

… specific flags per review if they are not found under gcloud config

jlewi · 2018-11-02T17:30:45Z

scripts/kfctl.sh

@@ -49,13 +49,64 @@ createEnv() {
      echo KUBEFLOW_DOCKER_REGISTRY=registry.aliyuncs.com >> ${ENV_FILE}
      ;;
    gcp)
-      PROJECT=${PROJECT:-$(gcloud config get-value project 2>/dev/null)}
+      while [[ $# -gt 0 ]]; do


Thanks for doing this.

Do you think we could support the syntax?
--name value
as well as
--name=value

That could be a separate PR.

We can @jlewi , I just have to put more logic to cover both scenarios 🙂
Feel free to file the ticket and describe it the way we want it to be done and assign it to me and I'll pick it up after this one.
But first let's get the tests fixed🤞

jlewi · 2018-11-02T17:34:32Z

Looks good other than failing test.

…-310

jlewi · 2018-11-05T14:08:26Z

scripts/kfctl.sh

+      --platform)
+        shift
+        PLATFORM=$1
+        mkdir -p ${DEPLOYMENT_NAME}


Why did you move the mkdir and other commands here and execute them as part of command line processing as opposed to executing them after command line parsing is done?

Because this DEPLOYMENT_NAME is really only used by gcp specific init calls and Also I needed to be inside that directory to make createEnv $* call and then later parse args.
So mkdir needs to be called either here or within createEnv function. 🙂

I"m still not following.

Should the instructions

mkdir -p ${DEPLOYMENT_NAME}
cd ${DEPLOYMENT}
createEnv

be executed for all platforms not just GCP?

The directory ${DEPLOYMENT_NAME} is used as the top level directory for all the Kubeflow related config (e.g. env.sh) so I don't think its GCP specific.

I see you're passing the command line arguments back to createEnv and then reparsing them there to persist them.

Can we just set a dictionary of environment variables here and then pass that to createEnv? That seems a little cleaner then parsing command line arguments in two places. We can also avoid putting logic in the argument parsing case statements.

You mean do the whole arg parsing business here, and then pass them to createEnv with the parsed values?

We can but do we want that in this PR or a subsequent PR.

I was thinking of doing one PR to just go over all the shell scripts and enhance path detection, format and style, enhance user messages specially during failures and that sort of thing.

Let me know what you think.

Yes that's what I meant. Specifically; I'd like to do flag parsing in a single location. Otherwise we will likely have problems later on because we update one location but not the other.

I'm ok with doing this in a follow on PR as long as you think you can to it in the next week or so. I'll LGTM this with a hold. If you want to submit this first and fix command line parsing later just remove the hold.

Thanks @jlewi
I'll do the next PR sooner than later this week.
I'll file a ticket and go from there.

jlewi · 2018-11-05T21:55:24Z

.gitignore

@@ -37,6 +37,8 @@ examples/.ipynb_checkpoints/

 **/.ipynb_checkpoints

+# env.sh generated by kfct.sh


kfctl.sh

Missing .sh

jlewi · 2018-11-05T23:13:49Z

/hold (see comment and decide whether to remove the hold)
/lgtm
/approve

k8s-ci-robot · 2018-11-05T23:13:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jlewi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…ix styles for kfctl.sh (kubeflow#1888) * Provide an error message in case GCP ZONE is not set and exit, also fix styles for kfctl.sh * Provide an error message in case GCP ZONE is not set and exit, also fix styles for kfctl.sh * provide zone info for kfctl_test.jsonnet as well * Add parameter option for --zone * Move the GCP specific flags up under GCP env creation, mandate gcloud specific flags per review if they are not found under gcloud config * source env.sh prior to calling gcpInitProject * move check for SKIP_INIT_PROJECT back to main * Minor style fix * Fix the missing ENVs during deploy and adding env.sh to .gitignore * Attempt to make kfct.sh executable * Fix a typo in .gitignore per review feedback

* We want to enable the stackdriver monitoring agents by default. This allows pod logs to be fetched by pod label which is very valuable. * To enable this we need to use the v1beta1 API for GKE https://cloud.google.com/kubernetes-engine/docs/reference/api-organization#beta * Remove the field CLUSTER_VERSION from kfctl.sh This was added in kubeflow#1888 but it doesn't look like it was actually being use to set the CLUSTER_VERSION; it looks like cluster-version is set in the deployment manager config cluster-kubeflow.yaml Related to kubeflow#1757

* We want to enable the stackdriver monitoring agents by default. This allows pod logs to be fetched by pod label which is very valuable. * To enable this we need to use the v1beta1 API for GKE https://cloud.google.com/kubernetes-engine/docs/reference/api-organization#beta * Remove the field CLUSTER_VERSION from kfctl.sh This was added in #1888 but it doesn't look like it was actually being use to set the CLUSTER_VERSION; it looks like cluster-version is set in the deployment manager config cluster-kubeflow.yaml Related to #1757

…ix styles for kfctl.sh (kubeflow#1888) * Provide an error message in case GCP ZONE is not set and exit, also fix styles for kfctl.sh * Provide an error message in case GCP ZONE is not set and exit, also fix styles for kfctl.sh * provide zone info for kfctl_test.jsonnet as well * Add parameter option for --zone * Move the GCP specific flags up under GCP env creation, mandate gcloud specific flags per review if they are not found under gcloud config * source env.sh prior to calling gcpInitProject * move check for SKIP_INIT_PROJECT back to main * Minor style fix * Fix the missing ENVs during deploy and adding env.sh to .gitignore * Attempt to make kfct.sh executable * Fix a typo in .gitignore per review feedback

* We want to enable the stackdriver monitoring agents by default. This allows pod logs to be fetched by pod label which is very valuable. * To enable this we need to use the v1beta1 API for GKE https://cloud.google.com/kubernetes-engine/docs/reference/api-organization#beta * Remove the field CLUSTER_VERSION from kfctl.sh This was added in kubeflow#1888 but it doesn't look like it was actually being use to set the CLUSTER_VERSION; it looks like cluster-version is set in the deployment manager config cluster-kubeflow.yaml Related to kubeflow#1757

Provide an error message in case GCP ZONE is not set and exit, also f…

c3ef784

…ix styles for kfctl.sh

googlebot added the cla: yes label Oct 30, 2018

k8s-ci-robot requested review from abhi-g and pdmack October 30, 2018 06:48

k8s-ci-robot added the size/M label Oct 30, 2018

k8s-ci-robot assigned jlewi Oct 30, 2018

ashahba added 3 commits October 29, 2018 23:51

Provide an error message in case GCP ZONE is not set and exit, also f…

0a42e81

…ix styles for kfctl.sh

provide zone info for kfctl_test.jsonnet as well

976a5f7

Merge branch 'ashahba/fix-310' of github.com:ashahba/kubeflow into as…

d5ebb22

…hahba/fix-310

ashahba added 2 commits October 30, 2018 14:48

Merge branch 'master' of github.com:ashahba/kubeflow into ashahba/fix…

071b084

…-310

Add parameter option for --zone

67ba249

jlewi reviewed Oct 31, 2018

View reviewed changes

ashahba added 2 commits November 1, 2018 20:47

Merge branch 'master' of github.com:ashahba/kubeflow into ashahba/fix…

5156e7a

…-310

Move the GCP specific flags up under GCP env creation, mandate gcloud…

26925d0

… specific flags per review if they are not found under gcloud config

k8s-ci-robot added size/L and removed size/M labels Nov 2, 2018

ashahba added 3 commits November 1, 2018 23:33

source env.sh prior to calling gcpInitProject

d69b1d8

move check for SKIP_INIT_PROJECT back to main

644e60e

Minor style fix

bd5a4c0

jlewi reviewed Nov 2, 2018

View reviewed changes

ashahba added 2 commits November 3, 2018 21:43

Merge branch 'master' of github.com:ashahba/kubeflow into ashahba/fix…

c4c4f08

…-310

Fix the missing ENVs during deploy and adding env.sh to .gitignore

5246937

ashahba mentioned this pull request Nov 4, 2018

Fix skip init project; use spaces consistently for indentation #1902

Merged

jlewi reviewed Nov 5, 2018

View reviewed changes

Sync with master and resolve conflicts

d596240

Attempt to make kfct.sh executable

29460c7

jlewi reviewed Nov 5, 2018

View reviewed changes

Fix a typo in .gitignore per review feedback

019f75c

k8s-ci-robot added the lgtm label Nov 5, 2018

k8s-ci-robot added the approved label Nov 5, 2018

k8s-ci-robot merged commit 3ae451b into kubeflow:master Nov 5, 2018

ashahba mentioned this pull request Nov 12, 2018

Parse all command line options in one place and within function #1942

Merged

jlewi mentioned this pull request Dec 18, 2018

Enable stackdriver agents for GKE by default. #2118

Merged

ashahba deleted the ashahba/fix-310 branch February 10, 2019 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide an error message in case GCP ZONE is not set and exit, also fix styles for kfctl.sh #1888

Provide an error message in case GCP ZONE is not set and exit, also fix styles for kfctl.sh #1888

ashahba commented Oct 30, 2018 •

edited by jlewi

ashahba commented Oct 30, 2018

ashahba commented Oct 30, 2018

ashahba commented Oct 30, 2018

ashahba commented Oct 31, 2018

jlewi Oct 31, 2018

ashahba Oct 31, 2018 •

edited

jlewi Nov 1, 2018

jlewi Nov 2, 2018

ashahba Nov 2, 2018

jlewi commented Nov 2, 2018

jlewi Nov 5, 2018 •

edited

ashahba Nov 5, 2018

jlewi Nov 5, 2018

jlewi Nov 5, 2018

ashahba Nov 5, 2018

jlewi Nov 5, 2018

ashahba Nov 5, 2018

jlewi Nov 5, 2018

jlewi commented Nov 5, 2018

k8s-ci-robot commented Nov 5, 2018

		@@ -37,6 +37,8 @@ examples/.ipynb_checkpoints/

		**/.ipynb_checkpoints

		# env.sh generated by kfct.sh

Provide an error message in case GCP ZONE is not set and exit, also fix styles for kfctl.sh #1888

Provide an error message in case GCP ZONE is not set and exit, also fix styles for kfctl.sh #1888

Conversation

ashahba commented Oct 30, 2018 • edited by jlewi

ashahba commented Oct 30, 2018

ashahba commented Oct 30, 2018

ashahba commented Oct 30, 2018

ashahba commented Oct 31, 2018

Choose a reason for hiding this comment

ashahba Oct 31, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlewi commented Nov 2, 2018

jlewi Nov 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlewi commented Nov 5, 2018

k8s-ci-robot commented Nov 5, 2018

ashahba commented Oct 30, 2018 •

edited by jlewi

ashahba Oct 31, 2018 •

edited

jlewi Nov 5, 2018 •

edited