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

remove the non-generated client #35431

Merged
merged 2 commits into from
Oct 27, 2016
Merged

remove the non-generated client #35431

merged 2 commits into from
Oct 27, 2016

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Oct 24, 2016 •
edited by k8s-oncall
Loading

Removes the non-generated client from kube. The package has a few methods left, but nothing that needs updating when adding new groups.

@ingvagabund

This change is Reviewable

sttts
sttts reviewed Oct 24, 2016
View reviewed changes
pkg/client/typed/discovery/helper.go
_ "k8s.io/kubernetes/plugin/pkg/client/auth"
)

// MatchesServerVersion queries the server to compares the build version
Copy link
Contributor

Choose a reason for hiding this comment

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

s/compares/compare/

Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed, it's an old function moved here.

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Oct 24, 2016
sttts
sttts reviewed Oct 24, 2016
View reviewed changes
pkg/client/typed/discovery/helper.go
cVer := version.Get()
sVer, err := client.ServerVersion()
if err != nil {
return fmt.Errorf("couldn't read version from server: %v\n", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

no \n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are straight moves. I'm not sure what changing this will end up changing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, straight move minus the, "change the types to accept what you mean".

sttts
sttts reviewed Oct 24, 2016
View reviewed changes
pkg/client/typed/discovery/helper.go
// MatchesServerVersion queries the server to compares the build version
// (git hash) of the client with the server's build version. It returns an error
// if it failed to contact the server or if the versions are not an exact match.
func MatchesServerVersion(client DiscoveryInterface, c *restclient.Config) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

(bool, error) is the canonical return type here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are straight moves. I'm not sure what changing this will end up changing

@deads2k deads2k force-pushed the client-16-remove-old branch from cad5ff4 to a7cccb7 Compare October 24, 2016 14:35
@sttts
Copy link
Contributor

sttts commented Oct 24, 2016

Please split the file removal from the changes in the second commit.

ClientCache.discoveryClient is never written, only read. Would be worth to move ugly initialization in MatchesServerVersion to the ClientCache initialization. Or at least add a TODO.

@k8s-github-robot k8s-github-robot added the kind/design Categorizes issue or PR as related to design. label Oct 24, 2016
@sttts
Copy link
Contributor

sttts commented Oct 24, 2016

Otherwise, all the changes look mechanical. lgtm. You will win the most-lines-removed award for 1.5 with this.

@deads2k deads2k force-pushed the client-16-remove-old branch from a7cccb7 to c33c84e Compare October 24, 2016 14:58
@deads2k deads2k added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 24, 2016
@deads2k deads2k force-pushed the client-16-remove-old branch from c33c84e to 7dd818e Compare October 24, 2016 16:50
@deads2k deads2k added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 24, 2016
@ingvagabund
Copy link
Contributor

The helper's MatchesServerVersion and NegotiateVersion are perfect candidates for moving under discovery. Looks good.

@caesarxuchao
Copy link
Member

Good job!

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2016
@deads2k deads2k force-pushed the client-16-remove-old branch from 7dd818e to e20a9af Compare October 26, 2016 19:21
@deads2k deads2k added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 26, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit e20a9af2fec5197f97b74820a6bc7a6e52197982. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@deads2k deads2k force-pushed the client-16-remove-old branch from e20a9af to 81ae130 Compare October 26, 2016 20:10
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2016
@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2016
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5423eaf into kubernetes:master Oct 27, 2016
@gmarek gmarek mentioned this pull request Oct 27, 2016
Merged
@foxish foxish mentioned this pull request Oct 27, 2016
Merged
@yongtang yongtang mentioned this pull request Oct 27, 2016
Merged
xingzhou pushed a commit to xingzhou/kubernetes that referenced this pull request Dec 15, 2016
Merge pull request kubernetes#35431 from deads2k/client-16-remove-old
3c79627 
Automatic merge from submit-queue

remove the non-generated client

Removes the non-generated client from kube.  The package has a few methods left, but nothing that needs updating when adding new groups.

@ingvagabund
@deads2k deads2k deleted the client-16-remove-old branch February 1, 2017 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

Assignees

@sttts sttts

Labels
kind/design Categorizes issue or PR as related to design. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@deads2k @sttts @ingvagabund @caesarxuchao @k8s-ci-robot @k8s-github-robot @somejfn @googlebot