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

feat: Refactor to make it easy to extend new kinds #865

Merged
merged 2 commits into from
Oct 12, 2019

Conversation

gaocegege
Copy link
Member

@gaocegege gaocegege commented Oct 10, 2019

Signed-off-by: Ce Gao [email protected]

What this PR does / why we need it:

This PR introduces a new Interface Provider, to make it easy to add new kinds.

Special notes for your reviewer:

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:



This change is Reviewable

@gaocegege
Copy link
Member Author

/assign @johnugeorge @hougangliu

@hougangliu
Copy link
Member

/lgtm

@gaocegege
Copy link
Member Author

Addressed the comment, PTAL @johnugeorge

// Katib will inject metrics collector into master replica.
var JobRoleMap = map[string][]string{
// Job kind does not support distributed training, thus no master.
consts.JobKindJob: {},
Copy link
Member

Choose a reason for hiding this comment

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

@gaocegege Just wondering why don't we keep all job related constants in this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

SGTM

Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot do it since it will cause import cycle. If we defined consts in job/v1alpha3, job/v1alpha3 uses job/v1alph3/job, job/v1alph3/job uses consts defined in job/v1alpha3.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does it work for you? @johnugeorge

Copy link
Member

Choose a reason for hiding this comment

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

I feel that we have defined too many constants :)

@johnugeorge
Copy link
Member

Ready to be merged?

@gaocegege
Copy link
Member Author

I think so.

@hougangliu
Copy link
Member

/lgtm

@johnugeorge
Copy link
Member

/lgtm

@johnugeorge
Copy link
Member

/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge

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

@johnugeorge
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot merged commit 198a63a into kubeflow:master Oct 12, 2019
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