Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

GetDefaultContainerPortNumber to GetDefaultContainerPortName #41

Merged
merged 3 commits into from
Jun 12, 2019

Conversation

merlintang
Copy link
Contributor

@merlintang merlintang commented Jun 12, 2019

We need to get the port name of container for each job, so we would change the port number to port name in the job controller interface.


This change is Reviewable

@merlintang
Copy link
Contributor Author

@jian-he can you review this ?

@jian-he
Copy link
Contributor

jian-he commented Jun 12, 2019

LGTM

@merlintang
Copy link
Contributor Author

@gaocegege can you look at this as well ?

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

/lgtm

Could we keep the two functions? I think we may need the GetDefaultContainerPortNumber in the future.

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Looks good in general. Could you address @gaocegege’s comment?

@@ -62,7 +62,7 @@ type ControllerInterface interface {
GetDefaultContainerName() string

// Get the default container port number
Copy link
Member

Choose a reason for hiding this comment

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

Update the comment here

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jun 12, 2019
Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Member

@gaocegege gaocegege left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gaocegege

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 k8s-ci-robot merged commit 2648a68 into kubeflow:master Jun 12, 2019
@merlintang
Copy link
Contributor Author

@gaocegege @terrytangyuan @jian-he thanks for your comments.

@merlintang merlintang deleted the issue-40 branch June 12, 2019 04:08
@gaocegege
Copy link
Member

Thanks for your contribution! 🎉 👍

georgkaleido pushed a commit to georgkaleido/common that referenced this pull request Jun 9, 2022
…e in a specific Batch (kubeflow#41)

* Add script to query Danni for the number of valid Dans at a given date

* Add launcher for danni_count_batch.py
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants