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

Add component metadata #891

Merged
merged 21 commits into from
Mar 6, 2019
Merged

Add component metadata #891

merged 21 commits into from
Mar 6, 2019

Conversation

gaoning777
Copy link
Contributor

@gaoning777 gaoning777 commented Mar 1, 2019 •
edited by jlewi
Loading

This change is Reviewable

@gaoning777 gaoning777 requested a review from vicaire March 1, 2019 22:39
@gaoning777
add default in parameter
b0a7314
@gaoning777
remove the str restriction for the param default
57232e4
@gaoning777
add pipelinemeta
622ad68
@gaoning777
update param name in the check_type functions
10ceb09 
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
@gaoning777
Merge branch 'add-core-types-and-checking' into add-component-metadata
6084865
@gaoning777
remove default values for non-primitive types in the function signature
987e22f 
update the _check_valid_type_dict name
hongye-sun
hongye-sun reviewed Mar 5, 2019
View reviewed changes
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 overall, just had one question on the placement of the meta datatypes code.

sdk/python/kfp/dsl/_container_op.py Outdated Show resolved Hide resolved
@hongye-sun hongye-sun self-assigned this Mar 5, 2019
hongye-sun
hongye-sun reviewed Mar 5, 2019
View reviewed changes
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

@gaoning777 gaoning777 merged commit f6acbad into kubeflow:master Mar 6, 2019
@gaoning777 gaoning777 deleted the add-component-metadata branch March 6, 2019 00:02
cheyang pushed a commit to alibaba/pipelines that referenced this pull request Mar 28, 2019
@gaoning777
Add component metadata (kubeflow#891)
60517f7 
* add component metadata
Linchin pushed a commit to Linchin/pipelines that referenced this pull request Apr 11, 2023
@theofpa
add theofpa as infra-viewer (kubeflow#891)
ba65147
HumairAK pushed a commit to red-hat-data-services/data-science-pipelines that referenced this pull request Mar 11, 2024
@ScrapCodes
fix(controller): Added fields serviceAccountName and PodTemplate to P…
06487ac 
…ipelineLoop. (kubeflow#891)

* wip

* Added fields serviceAccountName and PodTemplate to PipelineLoop.

* review comment

* Update pipelineloop_types.go
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

@Ark-kun Ark-kun Ark-kun left review comments

@neuromage neuromage Awaiting requested review from neuromage

@vicaire vicaire Awaiting requested review from vicaire

Assignees

@hongye-sun hongye-sun

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

Successfully merging this pull request may close these issues.
4 participants
@gaoning777 @k8s-ci-robot @Ark-kun @hongye-sun