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.

Sign up for GitHub

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

Jump to bottom

Upgrade ksonnet to 0.13.1. #440

Closed
wants to merge 1 commit into from
Closed

Upgrade ksonnet to 0.13.1. #440

wants to merge 1 commit into from

Conversation

gyliu513
Copy link
Member

@gyliu513 gyliu513 commented Dec 3, 2018 •
edited by jlewi
Loading

Fixed #414

This change is Reviewable

@k8s-ci-robot
Copy link
Contributor

Hi @gyliu513. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

1 similar comment
@k8s-ci-robot
Copy link
Contributor

Hi @gyliu513. Thanks for your PR.

I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

IronPan
IronPan requested changes Dec 3, 2018
View reviewed changes
test/presubmit-tests-with-pipeline-deployment.sh
@@ -79,7 +79,7 @@ tar -czf "$local_code_archive_file" .
gsutil cp "$local_code_archive_file" "$remote_code_archive_uri"

# Install ksonnet
KS_VERSION="0.11.0"
KS_VERSION="0.13.1"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a requirement for install kubeflow
https://www.kubeflow.org/docs/started/getting-started

Copy link
Member

Choose a reason for hiding this comment

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

It's actually better that test uses minimum requirement to fail if earlier version is not working.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the problem if I upgrade to use the latest version of ksonnet?

Copy link
Member

Choose a reason for hiding this comment

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

We need to update the kubeflow document.

IronPan
IronPan requested changes Dec 3, 2018
View reviewed changes
pipeline/Dockerfile
@@ -7,7 +7,7 @@ RUN apt-get update -y
RUN apt-get install -y curl

# Install ksonnet
ARG KS_VERSION="0.11.0"
ARG KS_VERSION="0.13.1"
Copy link
Member

Choose a reason for hiding this comment

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

U sure this would cause any issue? there are integration test that covers deployment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please refer to the comments at #414 (comment) , I think the integration test is based on native Kubernetes but not other kubernetes distributions, like OpenShift, IBM Cloud Private etc.

Copy link
Member

Choose a reason for hiding this comment

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

This make sense. Thanks for the reference.

@IronPan
Copy link
Member

IronPan commented Dec 3, 2018

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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

1 similar comment
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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

@gyliu513
Copy link
Member Author

gyliu513 commented Dec 3, 2018

/test presubmit-e2e-test

@k8s-ci-robot
Copy link
Contributor

@gyliu513: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test presubmit-e2e-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

1 similar comment
@k8s-ci-robot
Copy link
Contributor

@gyliu513: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test presubmit-e2e-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Ark-kun
Copy link
Contributor

Ark-kun commented Dec 4, 2018

/test presubmit-e2e-test

@k8s-ci-robot
Copy link
Contributor

@gyliu513: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
presubmit-e2e-test 38a3689 link /test presubmit-e2e-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@Ark-kun
Copy link
Contributor

Ark-kun commented Dec 4, 2018

deployer failure looks persistent

@gyliu513
Copy link
Member Author

gyliu513 commented Dec 4, 2018

@Ark-kun if I add following to app.yaml, then deploy pipeline will succeed, this does not seem to be related to ks upgrade, is it a problem in current CI?

registries:
 ml-pipeline:
   protocol: github
   uri: github.com/kubeflow/pipelines/tree/master/pipeline

@Ark-kun
Copy link
Contributor

Ark-kun commented Dec 5, 2018

this does not seem to be related to ks upgrade

Why not? Maybe the new ks version is somehow more strict about its configuration.

@Ark-kun
Copy link
Contributor

Ark-kun commented Dec 7, 2018

/retest

@gyliu513
Copy link
Member Author

gyliu513 commented Dec 7, 2018

@Ark-kun let me check more, seems you are right, ks 0.13.1 is somehow more strict about its configuration.

@vicaire
Copy link
Contributor

vicaire commented Apr 24, 2019

Is this PR still needed? Looks like the last update was quite a while ago.

@gyliu513 gyliu513 closed this Apr 25, 2019
@gyliu513 gyliu513 deleted the ksonnet-upgrade branch April 25, 2019 00:47
Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023
@jlewi @k8s-ci-robot
Update label sync (kubeflow#447)
d57c167 
* kind/feature should also use improvement/enhancement as previous value
* Cleanup some of the area labels

Related to kubeflow#440
HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024
@fenglixa
feat(backend): Support dsl to define the pipeline loops via tekton (k…
3f750f1 
…ubeflow#440)

* enhance the when condition UI display

* Support dsl to define the pipeline loops via tekton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IronPan IronPan IronPan requested changes

@vicaire vicaire Awaiting requested review from vicaire

Assignees

@IronPan IronPan

Labels
approved lgtm needs-ok-to-test size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pipeline install timeout
5 participants
@gyliu513 @k8s-ci-robot @IronPan @Ark-kun @vicaire