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

Code-gen: Remove lowercasing for project imports #68484

Merged
merged 4 commits into from Oct 5, 2018

Conversation

@jsturtevant
Contributor

jsturtevant commented Sep 10, 2018

What this PR does / why we need it:
If a project has a uppercase letter in the project import path then the generated paths will end up in different directories when some of the files are lower-cased during generation. This PR fixes the few places where the imports are lower-cased.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/code-generator#20

Special notes for your reviewer:
There was another PR (#63892) that was started but closed with no explanation. It looks like it was missing a few spots area's where the lower casing was missed which are addressed in this PR.

I was not sure where or if there are tests for this section of code. Would be happy to add tests if you can point me in the right direction.

If you run the code-generator/generate-groups.sh against the project here you can see the lower-casing in action. The generated code is split across two directories and the project does not build but with these fixes code-generation completes as expected and the project builds.

Release note:

Code-gen: Remove lowercasing for project imports
@jsturtevant

This comment has been minimized.

Show comment
Hide comment
@jsturtevant

jsturtevant Sep 10, 2018

Contributor

/assign @caesarxuchao

Contributor

jsturtevant commented Sep 10, 2018

/assign @caesarxuchao

@yue9944882

This comment has been minimized.

Show comment
Hide comment
@yue9944882
Contributor

yue9944882 commented Sep 11, 2018

@caesarxuchao

This comment has been minimized.

Show comment
Hide comment
@caesarxuchao

caesarxuchao Sep 11, 2018

Member

/ok-to-test

Member

caesarxuchao commented Sep 11, 2018

/ok-to-test

@jsturtevant

This comment has been minimized.

Show comment
Hide comment
@jsturtevant

jsturtevant Sep 13, 2018

Contributor

/retest

Contributor

jsturtevant commented Sep 13, 2018

/retest

@jsturtevant

This comment has been minimized.

Show comment
Hide comment
@jsturtevant

jsturtevant Sep 13, 2018

Contributor

@caesarxuchao I was able to pass the tests with a few updates.

I started to add an example of the Uppercasing in _examples like the previous PR. The generation works as expected (I haven't commited this yet).

But during that further testing I notice that the verify tests don't actually run the smoke tests in the verify script. From the test output of this PR:

I0913 00:03:18.965] Smoke testing _example by compiling...
I0913 00:03:19.008] Building client-gen
W0913 00:03:19.108] warning: "vendor/k8s.io/code-generator/_example/..." matched no packages
W0913 00:03:19.109] warning: "vendor/k8s.io/code-generator/_example/..." matched no packages

There _example folder looks to have been renamed to _examples at some point. I made an update to build the projects by updating staging/src/k8s.io/code-generator/hack/verify-codegen.sh to build the samples for example:

go build ${SCRIPT_ROOT}/_examples/crd/...

but now I get the error:

./hack/verify-codegen.sh
Generating deepcopy funcs
Generating defaulters
Generating conversions
Generating clientset for example:v1 example2:v1 at k8s.io/code-generator/_examples/apiserver/clientset
Generating listers for example:v1 example2:v1 at k8s.io/code-generator/_examples/apiserver/listers
Generating informers for example:v1 example2:v1 at k8s.io/code-generator/_examples/apiserver/informers
Generating deepcopy funcs
Generating clientset for example:v1 example2:v1 at k8s.io/code-generator/_examples/crd/clientset
Generating listers for example:v1 example2:v1 at k8s.io/code-generator/_examples/crd/listers
Generating informers for example:v1 example2:v1 at k8s.io/code-generator/_examples/crd/informers
Generating deepcopy funcs
Generating clientset for example:v1 at k8s.io/code-generator/_examples/UpperCaseProjectName/clientset
Generating listers for example:v1 at k8s.io/code-generator/_examples/UpperCaseProjectName/listers
Generating informers for example:v1 at k8s.io/code-generator/_examples/UpperCaseProjectName/informers
diffing ./hack/../_examples against freshly generated codegen
./hack/../_examples up to date.
Smoke testing _example by compiling...
./hack/..
../../../../vendor/k8s.io/code-generator/_examples/crd/clientset/versioned/typed/example/v1/clustertesttype.go:28:2: cannot find package "k8s.io/kubernetes/pkg/apis/autoscaling" in any of:
	/home/jstur/go/src/github.com/kubernetes/kubernetes/vendor/k8s.io/kubernetes/pkg/apis/autoscaling (vendor tree)
	/usr/local/go/src/k8s.io/kubernetes/pkg/apis/autoscaling (from $GOROOT)
	/home/jstur/go/src/k8s.io/kubernetes/pkg/apis/autoscaling (from $GOPATH)

Since the smoke tests have not been running it seems this has been an issue before this PR as you can see on this file in master. Looking at the vendor folder it seems that the all those packages are symlinked back into the main pkg folder. Although none of them have the prefix kubernetes. It seems that there might be another bug related to how those autoscaling imports are being generated.

Thoughts on how to proceed? I could check in the samples folder with the Uppercase as an example, get this merged and closed, then open a separate bug for the re-enabling the smoke tests and fixing the generating of the autoscaling package? What do you think?

Contributor

jsturtevant commented Sep 13, 2018

@caesarxuchao I was able to pass the tests with a few updates.

I started to add an example of the Uppercasing in _examples like the previous PR. The generation works as expected (I haven't commited this yet).

But during that further testing I notice that the verify tests don't actually run the smoke tests in the verify script. From the test output of this PR:

I0913 00:03:18.965] Smoke testing _example by compiling...
I0913 00:03:19.008] Building client-gen
W0913 00:03:19.108] warning: "vendor/k8s.io/code-generator/_example/..." matched no packages
W0913 00:03:19.109] warning: "vendor/k8s.io/code-generator/_example/..." matched no packages

There _example folder looks to have been renamed to _examples at some point. I made an update to build the projects by updating staging/src/k8s.io/code-generator/hack/verify-codegen.sh to build the samples for example:

go build ${SCRIPT_ROOT}/_examples/crd/...

but now I get the error:

./hack/verify-codegen.sh
Generating deepcopy funcs
Generating defaulters
Generating conversions
Generating clientset for example:v1 example2:v1 at k8s.io/code-generator/_examples/apiserver/clientset
Generating listers for example:v1 example2:v1 at k8s.io/code-generator/_examples/apiserver/listers
Generating informers for example:v1 example2:v1 at k8s.io/code-generator/_examples/apiserver/informers
Generating deepcopy funcs
Generating clientset for example:v1 example2:v1 at k8s.io/code-generator/_examples/crd/clientset
Generating listers for example:v1 example2:v1 at k8s.io/code-generator/_examples/crd/listers
Generating informers for example:v1 example2:v1 at k8s.io/code-generator/_examples/crd/informers
Generating deepcopy funcs
Generating clientset for example:v1 at k8s.io/code-generator/_examples/UpperCaseProjectName/clientset
Generating listers for example:v1 at k8s.io/code-generator/_examples/UpperCaseProjectName/listers
Generating informers for example:v1 at k8s.io/code-generator/_examples/UpperCaseProjectName/informers
diffing ./hack/../_examples against freshly generated codegen
./hack/../_examples up to date.
Smoke testing _example by compiling...
./hack/..
../../../../vendor/k8s.io/code-generator/_examples/crd/clientset/versioned/typed/example/v1/clustertesttype.go:28:2: cannot find package "k8s.io/kubernetes/pkg/apis/autoscaling" in any of:
	/home/jstur/go/src/github.com/kubernetes/kubernetes/vendor/k8s.io/kubernetes/pkg/apis/autoscaling (vendor tree)
	/usr/local/go/src/k8s.io/kubernetes/pkg/apis/autoscaling (from $GOROOT)
	/home/jstur/go/src/k8s.io/kubernetes/pkg/apis/autoscaling (from $GOPATH)

Since the smoke tests have not been running it seems this has been an issue before this PR as you can see on this file in master. Looking at the vendor folder it seems that the all those packages are symlinked back into the main pkg folder. Although none of them have the prefix kubernetes. It seems that there might be another bug related to how those autoscaling imports are being generated.

Thoughts on how to proceed? I could check in the samples folder with the Uppercase as an example, get this merged and closed, then open a separate bug for the re-enabling the smoke tests and fixing the generating of the autoscaling package? What do you think?

@jsturtevant

This comment has been minimized.

Show comment
Hide comment
@jsturtevant

jsturtevant Sep 21, 2018

Contributor

@caesarxuchao all the tests pass is this good to merge? I can open issues for the comments above separately.

Contributor

jsturtevant commented Sep 21, 2018

@caesarxuchao all the tests pass is this good to merge? I can open issues for the comments above separately.

@caesarxuchao

Can you add a test demonstrating the client-gen works for camelcased path now?

@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/S labels Sep 24, 2018

@jsturtevant

This comment has been minimized.

Show comment
Hide comment
@jsturtevant

jsturtevant Sep 24, 2018

Contributor

I added an example with MixedCase. Also needed to update the verify script to build the packages correctly as stated in my previous comment.

Once the example projects were building properly, this uncovered a bug in the code generation for the sub resource scale in the existing crd example:

Smoke testing _example by compiling...
# k8s.io/kubernetes/vendor/k8s.io/code-generator/_examples/crd/clientset/versioned/typed/example/v1/fake
vendor/k8s.io/code-generator/_examples/crd/clientset/versioned/typed/example/v1/fake/fake_clustertesttype.go:137:46: not enough arguments in call to testing.NewRootGetSubresourceAction
	have (schema.GroupVersionResource, string)
	want (schema.GroupVersionResource, string, string)

I fixed this in the generator_fake_for_type.go by adding the correct parameters. Everything builds nows. I also tested the updates against my larger project that prompted this bug fix as mentioned in the initial PR.

Contributor

jsturtevant commented Sep 24, 2018

I added an example with MixedCase. Also needed to update the verify script to build the packages correctly as stated in my previous comment.

Once the example projects were building properly, this uncovered a bug in the code generation for the sub resource scale in the existing crd example:

Smoke testing _example by compiling...
# k8s.io/kubernetes/vendor/k8s.io/code-generator/_examples/crd/clientset/versioned/typed/example/v1/fake
vendor/k8s.io/code-generator/_examples/crd/clientset/versioned/typed/example/v1/fake/fake_clustertesttype.go:137:46: not enough arguments in call to testing.NewRootGetSubresourceAction
	have (schema.GroupVersionResource, string)
	want (schema.GroupVersionResource, string, string)

I fixed this in the generator_fake_for_type.go by adding the correct parameters. Everything builds nows. I also tested the updates against my larger project that prompted this bug fix as mentioned in the initial PR.

@jsturtevant

This comment has been minimized.

Show comment
Hide comment
@jsturtevant

jsturtevant Sep 25, 2018

Contributor

@caesarxuchao the latest commits passed the tests. Ready for review. Thanks!

Contributor

jsturtevant commented Sep 25, 2018

@caesarxuchao the latest commits passed the tests. Ready for review. Thanks!

@caesarxuchao

This comment has been minimized.

Show comment
Hide comment
@caesarxuchao

caesarxuchao Sep 25, 2018

Member

LGTM. Thanks. Can you squash it to meaningful commits?

Member

caesarxuchao commented Sep 25, 2018

LGTM. Thanks. Can you squash it to meaningful commits?

jsturtevant added some commits Sep 10, 2018

Remove lowercasing for project imports
This commit provides a fix for the scenario where a project has an
uppercase letter in the project import path. Prior to this fix
the generated files would end up in different directories with some
of the imports being lower-cased during generation. An example of this would
be a project such as 'github.com/MixedCase/project' would result in
some of the imports with 'github.com/mixedcase/project' causing a broken
build.
Re-enable smoke tests in verify-codegen script
The smoke tests were not being run for the example projects.  Re-enabled
the smoke tests by building each of the sample projects.
Fix code generation by adding missing parameter
The code generation for fake types was missing the subresource path
parameter in the template which caused a compile error for the
sample projects using the scale subresource.  Also re-ran the code
generation after applying the fix.
@jsturtevant

This comment has been minimized.

Show comment
Hide comment
@jsturtevant

jsturtevant Sep 26, 2018

Contributor

@caesarxuchao squashed commits and tests passed. Looks ready to go. Thanks again!

Contributor

jsturtevant commented Sep 26, 2018

@caesarxuchao squashed commits and tests passed. Looks ready to go. Thanks again!

@caesarxuchao

This comment has been minimized.

Show comment
Hide comment
@caesarxuchao

caesarxuchao Sep 26, 2018

Member

/lgtm

Member

caesarxuchao commented Sep 26, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Sep 26, 2018

@caesarxuchao

This comment has been minimized.

Show comment
Hide comment
@caesarxuchao

caesarxuchao Sep 26, 2018

Member

/assign @lavalamp

for approval

Member

caesarxuchao commented Sep 26, 2018

/assign @lavalamp

for approval

@nikhita

This comment has been minimized.

Show comment
Hide comment
@nikhita

nikhita Sep 29, 2018

Member

/assign @sttts
/kind cleanup

Member

nikhita commented Sep 29, 2018

/assign @sttts
/kind cleanup

go build ./${SCRIPT_ROOT}/_examples/crd/...
go build ./${SCRIPT_ROOT}/_examples/apiserver/...
go build ./${SCRIPT_ROOT}/_examples/MixedCase/...

This comment has been minimized.

@sttts

sttts Oct 5, 2018

Contributor

👍

@sttts

sttts Oct 5, 2018

Contributor

👍

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Oct 5, 2018

Contributor

/lgtm
/approve

Contributor

sttts commented Oct 5, 2018

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Oct 5, 2018

Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, jsturtevant, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Contributor

k8s-ci-robot commented Oct 5, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, jsturtevant, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 5b9eb1c into kubernetes:master Oct 5, 2018

18 checks passed

cla/linuxfoundation jsturtevant authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment