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.

Sign up for GitHub

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

Jump to bottom

Move install of version handler to genericapiserver #24774

Merged
merged 1 commit into from Apr 27, 2016
Merged

Move install of version handler to genericapiserver #24774

merged 1 commit into from Apr 27, 2016

Conversation

jianhuiz
Copy link
Contributor

This is to satisfy kbuectl verification

Please review only the last commit.
#19313 #23653

@nikhiljindal @quinton-hoole, @deepak-vij, @XiaoningDing, @alfred-huangjian @mfanjie @huangyuqi @colhom

@k8s-bot
Copy link

k8s-bot commented Apr 26, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Apr 26, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 26, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

1 similar comment
@k8s-bot
Copy link

k8s-bot commented Apr 26, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in hack/jenkins/job-configs/kubernetes-jenkins-pull/ instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@ghost ghost added this to the v1.3 milestone Apr 26, 2016
@ghost ghost assigned nikhiljindal and unassigned smarterclayton Apr 26, 2016
@ghost ghost added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 26, 2016
@ghost
Copy link

ghost commented Apr 26, 2016

@k8s-bot ok to test

@nikhiljindal nikhiljindal added the release-note-none Denotes a PR that doesn't merit a release note. label Apr 27, 2016
@nikhiljindal
Copy link
Contributor

hack/verify-govet.sh failed which I assume is due to #23847.

You dont really need the commits in #23847 to be able to make this change, right?

@nikhiljindal
Copy link
Contributor

This still duplicates the call in both federated-apiserver and master.go.
How about splitting InstallSupport() into InstallVersionHandler and InstallHealthChecks?
Then we can call InstallVersionHandler (which is needed by both) in genericapiserver and call InstallHealthChecks only in master.go.

@jianhuiz
Copy link
Contributor Author

the test requires TestRun() in server_test.go which was added in #23847

@jianhuiz
Copy link
Contributor Author

Do you want me to do it in this one or will you create a new PR?

This still duplicates the call in both federated-apiserver and master.go.
How about splitting InstallSupport() into InstallVersionHandler and InstallHealthChecks?
Then we can call InstallVersionHandler (which is needed by both) in genericapiserver and call InstallHealthChecks only in master.go.

@nikhiljindal
Copy link
Contributor

Feel free to do it in this one.

@jianhuiz jianhuiz changed the title Register support handler (/version) to federated-apiserver Move install of version handler to genericapiserver Apr 27, 2016
@k8s-github-robot k8s-github-robot added needs-ok-to-merge needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 27, 2016
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 27, 2016
@nikhiljindal
Copy link
Contributor

LGTM, thanks!

@nikhiljindal nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Apr 27, 2016

GCE e2e build/test passed for commit fdfe42e.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit d89b6bc into kubernetes:master Apr 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikhiljindal nikhiljindal

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jianhuiz @k8s-bot @nikhiljindal @k8s-github-robot @smarterclayton @googlebot