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

Updated the "Basic - Exit handler" sample #1109

Merged
merged 1 commit into from
Apr 12, 2019
Merged

Updated the "Basic - Exit handler" sample #1109

merged 1 commit into from
Apr 12, 2019

Conversation

Ark-kun
Copy link
Contributor

@Ark-kun Ark-kun commented Apr 9, 2019 •
edited by jlewi
Loading

Modernized the sample pipeline code.
/assign @gaoning777 @hongye-sun
/label area/components

This change is Reviewable

@Ark-kun
Updated the "Basic - Exit handler" sample
33e18bb 
Modernized the sample pipeline code.
@Ark-kun Ark-kun requested review from gaoning777 and hongye-sun April 9, 2019 00:03
@gaoning777
Copy link
Contributor

Thanks for updating these basic samples.
If I may, can I request you to combine the PRs that update the samples into one PR, please?
Thanks

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Apr 9, 2019

The sample tests have passed. https://prow.k8s.io/pr-history/?org=kubeflow&repo=pipelines&pr=1109

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Apr 11, 2019

Thanks for updating these basic samples.
If I may, can I request you to combine the PRs that update the samples into one PR, please?
Thanks

I got feedback that I should make try and make my PRs smaller and more independent/granular.
This way it's easier to review each PR and also an issue in one PR is not blocking other PRs.

I accept your new feedback and will try updating the samples all at once in future.

For these samples it would be best not to spend additional time on recombining the PRs.

@gaoning777
Copy link
Contributor

Discussed offline.
For bigger changes that apply the same logic change to many files, it might be better to put in one single PR to reduce the CR time.
For even small changes that apply multiple logic changes to a few files. it might be better to divide into multiple PRs.

The rule of thumb, IMHO, is to help with CR and bug detection.

@gaoning777
Copy link
Contributor

/lgtm

@Ark-kun
Copy link
Contributor Author

Ark-kun commented Apr 11, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

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
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ark-kun

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 Apr 11, 2019

/test kubeflow-pipeline-e2e-test

@k8s-ci-robot k8s-ci-robot merged commit 2d10492 into kubeflow:master Apr 12, 2019
@Ark-kun Ark-kun deleted the Updated-the-"Basic---Exit-handler"-sample branch April 22, 2019 23:18
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
@dependabot
Bump django from 3.0.5 to 3.0.7 in /docs/samples/explanation/aix/mnist (
dec9ae9 
kubeflow#1109)

Bumps [django](https://github.com/django/django) from 3.0.5 to 3.0.7.
- [Release notes](https://github.com/django/django/releases)
- [Commits](django/django@3.0.5...3.0.7)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaoning777 gaoning777 Awaiting requested review from gaoning777

@hongye-sun hongye-sun Awaiting requested review from hongye-sun

Assignees

@gaoning777 gaoning777

@hongye-sun hongye-sun

Labels
approved area/samples lgtm size/M
Projects
None yet
Milestone
No milestone
Development

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