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(group): Update CRD group to kubeflow.org #354

Merged
merged 2 commits into from
Jan 30, 2018
Merged

feat(group): Update CRD group to kubeflow.org #354

merged 2 commits into from
Jan 30, 2018

Conversation

gaocegege
Copy link
Member

@gaocegege gaocegege commented Jan 30, 2018

Close #337

Signed-off-by: Ce Gao ce.gao@outlook.com


This change is Reviewable

Signed-off-by: Ce Gao <ce.gao@outlook.com>
@k8s-ci-robot
Copy link

Hi @gaocegege. Thanks for your PR.

I'm waiting for a kubernetes or tensorflow member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@coveralls
Copy link

coveralls commented Jan 30, 2018

Coverage Status

Coverage increased (+0.1%) to 31.856% when pulling d4db8a5 on gaocegege:group into b0ea60e on tensorflow:master.

@jlewi
Copy link
Contributor

jlewi commented Jan 30, 2018

Thanks for doing this!
Looks like there are some lint issues but otherwise looks good.

@gaocegege
Copy link
Member Author

Thanks and I will fix it soon.

Signed-off-by: Ce Gao <ce.gao@outlook.com>
@gaocegege
Copy link
Member Author

PTAL @jlewi

@@ -34,27 +34,27 @@ type TFJobLister interface {
TFJobListerExpansion
}

// tfJobLister implements the TFJobLister interface.
type tfJobLister struct {
// tFJobLister implements the TFJobLister interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change the capitalization to "tFJobLister" this looks strange to me; I prefer tfJobLister. Since we decided on TFJob (i.e capitalizing TF) i think it makes sense to make both T & F lower case for non public variables.

Copy link
Contributor

@jlewi jlewi Jan 30, 2018

Choose a reason for hiding this comment

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

Either way if you do think the capitalization should be changed lets discuss in a different PR. Since this PR touches so many files it would be good to get it in soon.

Copy link
Member Author

@gaocegege gaocegege Jan 30, 2018

Choose a reason for hiding this comment

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

This is auto-changed by ./hack/update-codegen.sh, I will try to update it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should not change the code in client/ manually since it will be overrode next time we run the script, and the name is changed to tF since the job is renamed from TfJob to TFJob. #332

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. I didn't realize this was autogenerated.

@jlewi
Copy link
Contributor

jlewi commented Jan 30, 2018

/ok-to-test

@jlewi jlewi changed the title feat(group): Update to kubeflow.org feat(group): Update CRD group to kubeflow.org Jan 30, 2018
@jlewi jlewi merged commit 7f97cf0 into kubeflow:master Jan 30, 2018
@gaocegege gaocegege deleted the group branch January 30, 2018 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants