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

Pass meta to containerop and pipeline #905

Merged
merged 66 commits into from
Mar 6, 2019
Merged

Pass meta to containerop and pipeline #905

merged 66 commits into from
Mar 6, 2019

Conversation

gaoning777
Copy link
Contributor

@gaoning777 gaoning777 commented Mar 4, 2019

Pass the metadata from the component and pipeline to ContainerOp and Pipeline


This change is Reviewable

remove schema validators for GCRPath, and adjust for GCRPath, GCSPath
change _check_valid_dict to _check_valid_type_dict to avoid confusion
fix typo in the comments
adjust function order for readability
@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 6, 2019

GitHub diff seems to be glitching for me, so I'll write my commentt here:

For TypeMeta.to_dict():

We could make the result the same as in component yaml by doing the serialization like this:

def to_dict(self):
    if self.properties:
        return {self.name: self.properties}
    else:
        return self.name

But I'm fine with whatever you decide to do.

@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 6, 2019

Apart from my last comments, this looks pretty good to me.

@gaoning777
Copy link
Contributor Author

@Ark-kun I modified the TypeMeta to be serialized to both str and dict.

Copy link
Contributor

@hongye-sun hongye-sun left a comment

Choose a reason for hiding this comment

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

/lgtm

@gaoning777
Copy link
Contributor Author

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaoning777

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

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

@gaoning777 gaoning777 merged commit 974d602 into kubeflow:master Mar 6, 2019
@gaoning777 gaoning777 deleted the pass-meta-to-containerop branch March 6, 2019 21:42
@Ark-kun
Copy link
Contributor

Ark-kun commented Mar 6, 2019

/lgtm
/approve

cheyang pushed a commit to alibaba/pipelines that referenced this pull request Mar 28, 2019
pass metadata from python conf to containerop and the pipeline
HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024
* update tekton client to 0.31.4

* update tekton manifests

* add missing go mod files
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.

4 participants