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

v1alpha2: Add API and codegen #523

Merged
merged 7 commits into from
Apr 10, 2018
Merged

v1alpha2: Add API and codegen #523

merged 7 commits into from
Apr 10, 2018

Conversation

gaocegege
Copy link
Member

@gaocegege gaocegege commented Apr 10, 2018

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


This change is Reviewable

Signed-off-by: Ce Gao <[email protected]>
Signed-off-by: Ce Gao <[email protected]>
@gaocegege gaocegege changed the title WIP: v1alpha2: Add API and codegen v1alpha2: Add API and codegen Apr 10, 2018
@coveralls
Copy link

coveralls commented Apr 10, 2018

Coverage Status

Coverage increased (+1.8%) to 47.037% when pulling 87848e3 on gaocegege:api/v1alpha2 into 0704e24 on kubeflow:master.

@mitake
Copy link
Contributor

mitake commented Apr 10, 2018

@gaocegege this PR itself seems good to me. Do you already have tf-operator side changes?

@gaocegege
Copy link
Member Author

@mitake

We are going to split the changes into different PRs and this one is for API 😄

/assign @jlewi @mitake

Signed-off-by: Ce Gao <[email protected]>
@gaocegege gaocegege removed request for mitake and lluunn April 10, 2018 09:26
@mitake
Copy link
Contributor

mitake commented Apr 10, 2018

@gaocegege ok, then this PR looks good to me :)

@jlewi
Copy link
Contributor

jlewi commented Apr 10, 2018

Review status: 0 of 28 files reviewed at latest revision, all discussions resolved.


pkg/apis/tensorflow/v1alpha2/types.go, line 71 at r2 (raw file):

	// Restart policy for all TFReplicas within the TFJob.
	// One of Always, OnFailure, Never and ExitCode.
	// Default to Always.

Not sure a default of Always really make sense; you wouldn't want to restart the master on success. Could we make the policy required?
Or should we have a replica type dependent default? Having a default value that is replica dependent seems a little complicated thogh.


pkg/apis/tensorflow/v1alpha2/types.go, line 116 at r2 (raw file):

Represents

Nit "Represents" -> Conditions


pkg/apis/tensorflow/v1alpha2/utils.go, line 25 at r2 (raw file):

// Pformat returns a pretty format output of any value that can be marshalled to JSON.
func Pformat(value interface{}) string {

Why is this function in v1alpha2? It doesn't look like anything about this function is specific to a given version.
Can't we have a single Pformat function for all versions?


Comments from Reviewable

@gaocegege
Copy link
Member Author

gaocegege commented Apr 10, 2018

Not sure a default of Always really make sense; you wouldn't want to restart the master on success. Could we make the policy required?
Or should we have a replica type dependent default? Having a default value that is replica dependent seems a little complicated thogh.

This is defined in TFJobReplicasSpec and it should works on a single type. And I agree that always is weird and I think I could open an issue for it. After all PRs merged and I will fix it. WDYT

@gaocegege
Copy link
Member Author

Updated, PTAL

@gaocegege
Copy link
Member Author

Why is this function in v1alpha2? It doesn't look like anything about this function is specific to a given version.
Can't we have a single Pformat function for all versions?

I removed the function and use util package instead.

@jlewi
Copy link
Contributor

jlewi commented Apr 10, 2018

Review status: 0 of 27 files reviewed at latest revision, all discussions resolved.


pkg/apis/tensorflow/v1alpha2/types.go, line 71 at r2 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

Not sure a default of Always really make sense; you wouldn't want to restart the master on success. Could we make the policy required?
Or should we have a replica type dependent default? Having a default value that is replica dependent seems a little complicated thogh.

@gaocege commented elsewhere that will resolve this in a follow up issue.


Comments from Reviewable

@jlewi
Copy link
Contributor

jlewi commented Apr 10, 2018

/lgtm
/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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 792d399 into kubeflow:master Apr 10, 2018
@gaocegege gaocegege deleted the api/v1alpha2 branch April 11, 2018 01:47
@ddysher ddysher mentioned this pull request Apr 12, 2018
jetmuffin pushed a commit to jetmuffin/tf-operator that referenced this pull request Jul 9, 2018
* types.go: Add APIs

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

* apis: Related code

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

* update-codegen.sh: Update

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

* codegen: Update

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

* linter: Update

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

* types.go: Fix typo

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

* test: Remove Pformat

Signed-off-by: Ce Gao <[email protected]>
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.

5 participants