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

client-gen: stop embedding of GroupVersion client intfs #49370

Merged
merged 4 commits into from Aug 6, 2017
Merged

client-gen: stop embedding of GroupVersion client intfs #49370

merged 4 commits into from Aug 6, 2017

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Jul 21, 2017 •
edited

It is undefined (or at least uncontrollable) which methods of the clientset apigroup
interfaces are actually inherited. Moreover, there might be nameconflicts between the
accessors and inherited methods. This PR removes the embedding to make it unambiguous.

Enforce explicit references to API group client interfaces in clientsets to avoid ambiguity.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 21, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jul 21, 2017
@sttts sttts assigned gmarek and caesarxuchao and unassigned feiskyer Jul 21, 2017
@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jul 21, 2017
@gmarek
Copy link
Contributor

gmarek commented Jul 21, 2017

+100 for this PR:)

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2017
ash2k
ash2k reviewed Jul 21, 2017
View reviewed changes
cmd/kubelet/app/server.go Outdated
if err != nil {
glog.Warningf("Failed to create API Server client: %v", err)
}
eventClient = client.CoreV1()
Copy link
Member

Choose a reason for hiding this comment

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

This will panic with npe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@sttts sttts force-pushed the sttts-no-clientset-embedding branch from 4cc82f7 to 75480ba Compare July 21, 2017 11:59
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 21, 2017
sttts
sttts commented Jul 21, 2017
View reviewed changes
cmd/kube-proxy/app/server.go Outdated
@@ -413,7 +413,13 @@ func createClients(config componentconfig.ClientConnectionConfiguration, masterO
return nil, nil, err
}

<<<<<<< HEAD
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rebase conflicts...

@sttts sttts force-pushed the sttts-no-clientset-embedding branch 4 times, most recently from fbec1f8 to df0e32a Compare July 21, 2017 12:57
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2017
@sttts sttts force-pushed the sttts-no-clientset-embedding branch from df0e32a to 60a3e18 Compare July 26, 2017 19:46
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2017
@sttts sttts force-pushed the sttts-no-clientset-embedding branch 3 times, most recently from c6a19f3 to 9f0f253 Compare July 27, 2017 21:32
@caesarxuchao
Copy link
Member

Could you make a release note since it's a non-compatibility change?

caesarxuchao
caesarxuchao reviewed Jul 27, 2017
View reviewed changes
staging/src/k8s.io/kube-gen/cmd/client-gen/generators/generator_for_clientset.go Outdated
@@ -69,8 +69,15 @@ func (g *genClientset) GenerateType(c *generator.Context, t *types.Type, w io.Wr
// perhaps we can adapt the go2ild framework to this kind of usage.
sw := generator.NewSnippetWriter(w, c, "$", "$")

allGroups := clientgentypes.ToGroupVersionPackages(g.groups)
type groupArg struct {
Copy link
Member

Choose a reason for hiding this comment

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

Could you just add LowerCaseGroupVersion to GroupVersionPackage and update ToGroupVersionPackages? It seems only client-gen uses them.

Copy link
Member

Choose a reason for hiding this comment

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

@sttts how about this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@sttts sttts force-pushed the sttts-no-clientset-embedding branch from 76b4988 to 3ccdb54 Compare August 4, 2017 06:02
@sttts sttts 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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 4, 2017
@kow3ns kow3ns mentioned this pull request Aug 4, 2017
Closed
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2017
@sttts sttts force-pushed the sttts-no-clientset-embedding branch from 3ccdb54 to 0162eca Compare August 5, 2017 12:52
@sttts sttts 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. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 5, 2017
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

7 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@sttts sttts force-pushed the sttts-no-clientset-embedding branch from 0162eca to 3b310d8 Compare August 6, 2017 13:32
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2017
@sttts sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2017
@sttts
Copy link
Contributor Author

sttts commented Aug 6, 2017

/retest

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 49370, 49481)

@k8s-github-robot k8s-github-robot merged commit 979c86f into kubernetes:master Aug 6, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 6, 2017 •
edited

@sttts: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce 3b310d8 link /test pull-kubernetes-federation-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ash2k ash2k ash2k left review comments

@caesarxuchao caesarxuchao caesarxuchao left review comments

Assignees

@smarterclayton smarterclayton

@gmarek gmarek

@caesarxuchao caesarxuchao

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@sttts @gmarek @caesarxuchao @smarterclayton @k8s-github-robot @fejta-bot @k8s-ci-robot @ash2k @feiskyer