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

*: Implement the List interface for TfJobList #278

Merged
merged 3 commits into from
Jan 11, 2018
Merged

*: Implement the List interface for TfJobList #278

merged 3 commits into from
Jan 11, 2018

Conversation

gaocegege
Copy link
Member

@gaocegege gaocegege commented Jan 10, 2018

Notice: The PR is blocked by #276 , I will rebase after it is merged and remove DO NOT MERGE

The error is caused by the field name in TfJobList:

// TfJobList is a list of TfJobs clusters.
type TfJobList struct {
	metav1.TypeMeta `json:",inline"`
	// Standard list metadata
	// More info: http://releases.k8s.io/HEAD/docs/devel/api-conventions.md#metadata
	Metadata metav1.ListMeta `json:"metadata,omitempty"`
	// Items is a list of TfJobs
	Items []TfJob `json:"items"`
}

The name of field Metadata should be anonymous field.

ERROR: logging before flag.Parse: E0110 16:52:24.998866    3707 runtime.go:66] Observed a panic: &errors.errorString{s:"*v1alpha1.TfJobList does not implement the List interface"} (*v1alpha1.TfJobList does not implement the List interface)

This change is Reviewable

@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 10, 2018

Coverage Status

Coverage increased (+0.3%) to 30.729% when pulling 9bc8275 on gaocegege:list into 9c93d5f on tensorflow:master.

@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Coverage increased (+0.2%) to 30.624% when pulling 5b3d8ea on gaocegege:list into 9c93d5f on tensorflow:master.

jlewi pushed a commit that referenced this pull request Jan 10, 2018
* The kind should be lower case; this error was surfaced while trying to implement the list interface #278 

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

gaocegege commented Jan 10, 2018

In this PR, I removed the field name here and updated the client. Then the golinter returns warnings for deepcopy.go. I have to add generated deepcopy file into ignore here

@gaocegege
Copy link
Member Author

#276 was merged. Then I will reset the commits and re-push the changes tomorrow(I am in Shanghai and it is 23:00 now) to be more human-readable.

Signed-off-by: Ce Gao <ce.gao@outlook.com>
Signed-off-by: Ce Gao <ce.gao@outlook.com>
Signed-off-by: Ce Gao <ce.gao@outlook.com>
@gaocegege gaocegege changed the title DO NOT MERGE: Make TfJobList implement the List interface *: Implement the List interface for TfJobList Jan 11, 2018
@coveralls
Copy link

coveralls commented Jan 11, 2018

Coverage Status

Coverage increased (+0.07%) to 30.624% when pulling d9eae19 on gaocegege:list into adc8ab9 on tensorflow:master.

@gaocegege
Copy link
Member Author

gaocegege commented Jan 11, 2018

I reassembled the commit, and PTAL 😄

@jlewi
Copy link
Contributor

jlewi commented Jan 11, 2018

/ok-to-test

@jlewi
Copy link
Contributor

jlewi commented Jan 11, 2018

/test all

@jlewi jlewi merged commit 7ddafd5 into kubeflow:master Jan 11, 2018
@gaocegege gaocegege deleted the list branch January 11, 2018 05:15
@jlewi jlewi mentioned this pull request Jan 11, 2018
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