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

Adding --skipInitProject to kfctl_test.jsonnet until CI quota is increased #1564

Merged
merged 11 commits into from Sep 27, 2018

Conversation

ashahba
Copy link
Member

@ashahba ashahba commented Sep 19, 2018

fixes #1562


This change is Reviewable

@gaocegege gaocegege removed their request for review September 19, 2018 05:01
@ashahba
Copy link
Member Author

ashahba commented Sep 19, 2018

@jlewi did we want this in?

@jlewi
Copy link
Contributor

jlewi commented Sep 24, 2018

@ashahba Apologies for the delay. (I also recommend doing /assign if you want someone to review your PRs) that way it shows up in (https://k8s-gubernator.appspot.com/pr) as needing attention.

I'll take a look

@jlewi
Copy link
Contributor

jlewi commented Sep 24, 2018

I think this is a good fix; even when quota fixes are resolved.

I'll open up an issue to properly test the uninitialized project case.

@jlewi
Copy link
Contributor

jlewi commented Sep 24, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot removed the lgtm label Sep 24, 2018
@ashahba
Copy link
Member Author

ashahba commented Sep 24, 2018

/assign @jlewi

@jlewi
Copy link
Contributor

jlewi commented Sep 24, 2018

/ok-to-test

@jlewi
Copy link
Contributor

jlewi commented Sep 24, 2018

@ashahba sent you an invite to the org so you can trigger the tests on your PRs.

@ashahba
Copy link
Member Author

ashahba commented Sep 24, 2018

/retest

@ashahba
Copy link
Member Author

ashahba commented Sep 25, 2018

/retest

@jlewi
Copy link
Contributor

jlewi commented Sep 26, 2018

The presubmit failed.

Looks like there might be a bug in kfctl.s

+ echo CONFIG_FILE=cluster-kubeflow.yaml
+ '[' -z '' ']'
++ gcloud projects describe --skipInitProject '--format=value(project_number)'
ERROR: (gcloud.projects.describe) unrecognized arguments: --skipInitProject (did you mean '--project'?)
Usage: gcloud projects describe PROJECT_ID [optional flags]
 optional flags may be  --help
For detailed information on this command and its flags, run:
 gcloud projects describe --help

Looks like --skipInitProject is being added to the gcloud project describe command but it shouldn't be.

@jlewi
Copy link
Contributor

jlewi commented Sep 26, 2018

/assign @kunmingg

@kunming can you take the lead on reviewing this PR? I'm a bit backed up.

@ashahba
Copy link
Member Author

ashahba commented Sep 26, 2018

@kunmingg Jeremy is right, looks like we are passing irrelevant options to gcloud in kfctl.sh.
I was a bit busy last couple of days, I'll take a closer look to see where we mix up arguments.
But by all means feel free to provide your feedback.

@@ -156,6 +156,8 @@ local dagTemplates = [
"--platform",
"gcp",
"--project",
// Temporary fix for https://github.com/kubeflow/kubeflow/issues/1562
"--skipInitProject",
Copy link
Contributor

Choose a reason for hiding this comment

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

line 160 and 161 should swap.

@ashahba
Copy link
Member Author

ashahba commented Sep 26, 2018

Looks like we are still getting:

Command failed: /mnt/test-data-volume/kubeflow-presubmit-kfctl-1564-db10e7a-3535-baed/src/kubeflow/kubeflow/scripts/kfctl.sh init kfctl-b
aed --platform gcp --project kubeflow-ci --skipInitProject
command didn't succeed

🤔

@ashahba
Copy link
Member Author

ashahba commented Sep 27, 2018

@kunmingg and @jlewi turned out the while loop was making an extra shift near line https://github.com/kubeflow/kubeflow/pull/1564/files#diff-8a6f2d237f052b07ca594c20830ed9d2L111
which ended up loosing some command line options.

But it's fixed now and this PR is ready for merge 🙂

@jlewi
Copy link
Contributor

jlewi commented Sep 27, 2018

Thanks!
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot merged commit 3291703 into kubeflow:master Sep 27, 2018
@ashahba ashahba deleted the ashahba/fix-1562 branch September 27, 2018 20:25
saffaalvi pushed a commit to StatCan/kubeflow that referenced this pull request Feb 11, 2021
…eased (kubeflow#1564)

* Adding --skipInitProject to kfctl_test.jsonnet until CI quota is increased

* Added some inline comments for the --skipInitProject option in kfctl_test.jsonnet

* Fix jsonnet style in kfctl_test.jsonnet

* Attempt to delete GKE deployment only if KUBEFLOW_DM_DIR is present

* Fix the malformed cli options per review

* call gcpInitProject within kfctl.sh only if --skipInitProject is not provided

* Modified the while loop and assign SKIP_INIT_PROJECT=true

* Fix while loop numerical comparison in kfctl.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

presubmit build is failing with a quota error
5 participants