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

Enable pipeline packages with multiple files #939

Merged
merged 12 commits into from
Mar 28, 2019
Merged

Enable pipeline packages with multiple files #939

merged 12 commits into from
Mar 28, 2019

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Mar 7, 2019 •
edited
Loading

Old behavior:
Choose the first file in the archive (order is unknown) and then check if it has .yaml extension.

New behavior:
Go through the files in the archive and chose the first one whose name ends in pipeline.yaml.

Backwards compatibility considerations:
The DSL compiler has always generated .tar.gz files with the pipeline.yaml file inside. So all pipeline packages ever created by the DSL compiler are compatible with this change.

This change is Reviewable

@Ark-kun Ark-kun requested review from IronPan and hongye-sun March 7, 2019 23:43
@k8s-ci-robot k8s-ci-robot requested a review from vicaire March 7, 2019 23:43
@Ark-kun Ark-kun force-pushed the Enable-pipeline-packages-with-multiple-files branch 2 times, most recently from ffccc0d to 91be119 Compare March 8, 2019 00:33
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Mar 8, 2019
@Ark-kun Ark-kun force-pushed the Enable-pipeline-packages-with-multiple-files branch 2 times, most recently from 6087b49 to 7b745d2 Compare March 8, 2019 00:50
@Ark-kun Ark-kun force-pushed the Enable-pipeline-packages-with-multiple-files branch from 7b745d2 to 827be7a Compare March 8, 2019 22:31
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Mar 9, 2019

/test kubeflow-pipeline-e2e-test

1 similar comment
@Ark-kun
Copy link
Contributor Author

Ark-kun commented Mar 12, 2019

/test kubeflow-pipeline-e2e-test

@Ark-kun Ark-kun force-pushed the Enable-pipeline-packages-with-multiple-files branch from 0572b9d to 3d6de02 Compare March 19, 2019 18:46
@Ark-kun Ark-kun force-pushed the Enable-pipeline-packages-with-multiple-files branch from 28f2db3 to 35aa6dd Compare March 19, 2019 23:58
@Ark-kun Ark-kun changed the title [WIP]Enable pipeline packages with multiple files Enable pipeline packages with multiple files Mar 20, 2019
neuromage
neuromage suggested changes Mar 26, 2019
View reviewed changes
@Ark-kun
Addressed the PR feedback
dee8ddb 
Added space before comment.
Checking for the error again.
@Ark-kun Ark-kun requested a review from neuromage March 26, 2019 21:45
@neuromage
Copy link
Contributor

/lgtm

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Mar 27, 2019

/cc @IronPan @vicaire

@k8s-ci-robot k8s-ci-robot requested a review from IronPan March 27, 2019 01:13
@vicaire
Copy link
Contributor

vicaire commented Mar 27, 2019

@Ark-kun, as we agreed on yesterday, could you please add a file in the tar that provides:

  1. a version
  2. a description of the files included in the tar.

The organization of the tar file is an API to the system. It should have proper versioning instead of relying on convention like "what is the first file with a certain prefix".

Thanks.

@vicaire vicaire assigned vicaire and unassigned IronPan Mar 27, 2019
@vicaire
Copy link
Contributor

vicaire commented Mar 27, 2019

/hold

@hongye-sun
Copy link
Contributor

@vicaire

This fix is very important for AIHub and they are expecting this will be there in this week's release. Can we go with the convention approach for now and we will soon deprecate the component.yaml right after we support loading component from pipeline.yaml.

@vicaire
Copy link
Contributor

vicaire commented Mar 28, 2019

Sounds good to me. Thanks @hongye-sun

/approve
/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vicaire

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

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

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Mar 28, 2019

Removing the hold with @vicaire's approval.
/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 2c2445d into kubeflow:master Mar 28, 2019
@Ark-kun Ark-kun deleted the Enable-pipeline-packages-with-multiple-files branch March 29, 2019 21:44
@Ark-kun Ark-kun mentioned this pull request Mar 30, 2019
Merged
Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023
[Follow Up] Add PULL_BASE_REF for checking out repos (kubeflow#939)
0a2b1d0 
[Follow Up] Add PULL_BASE_REF for checking out repos
HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024
@yhwang
[build] update github workflow to use go v1.17 (kubeflow#939)
8f034ff 
the whole repo needs go v1.17 now.

Signed-off-by: Yihong Wang <yh.wang@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hongye-sun hongye-sun hongye-sun left review comments

@neuromage neuromage neuromage approved these changes

@vicaire vicaire Awaiting requested review from vicaire

@IronPan IronPan Awaiting requested review from IronPan

Assignees

@neuromage neuromage

@vicaire vicaire

@hongye-sun hongye-sun

Labels
approved area/backend lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Ark-kun @vicaire @hongye-sun @neuromage @k8s-ci-robot @IronPan