Code-gen: Remove lowercasing for project imports #68484
Conversation
k8s-ci-robot
added
release-note
|
/assign @caesarxuchao |
|
/cc @kubernetes/code-generator-maintainers |
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
jsturtevant
force-pushed
the
fix-lowercased-imports
branch
from
fd7217f
to
b692073
Compare
|
/retest |
|
@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: There but now I get the error: 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 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? |
|
@caesarxuchao all the tests pass is this good to merge? I can open issues for the comments above separately. |
staging/src/k8s.io/code-generator/cmd/client-gen/generators/client_generator.go
Outdated
Show resolved
Hide resolved
jsturtevant
force-pushed
the
fix-lowercased-imports
branch
from
a7da549
to
86ff5f8
Compare
k8s-ci-robot
added
size/XXL
|
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 I fixed this in the |
|
@caesarxuchao the latest commits passed the tests. Ready for review. Thanks! |
|
LGTM. Thanks. Can you squash it to meaningful commits? |
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.
The smoke tests were not being run for the example projects. Re-enabled the smoke tests by building each of the sample projects.
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
force-pushed
the
fix-lowercased-imports
branch
from
86ff5f8
to
93d8a53
Compare
|
@caesarxuchao squashed commits and tests passed. Looks ready to go. Thanks again! |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @lavalamp for approval |
|
/assign @sttts |
k8s-ci-robot
added
kind/cleanup
| go build ./${SCRIPT_ROOT}/_examples/crd/... | ||
| go build ./${SCRIPT_ROOT}/_examples/apiserver/... | ||
| go build ./${SCRIPT_ROOT}/_examples/MixedCase/... |
|
/lgtm |
|
[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 |
k8s-ci-robot
added
the
approved
The PR (kubernetes/kubernetes#68484) was merged into upstream kubernetes. This commit removes the temporary fix for lowercasing. The latest version of the code gen has an update to the patch client that use a parameter that is not in current version of Go-Client used by this repo so dropped code gen for patching. Patching is not being used currently so safe to drop. May consider dropping other verbs not used in the client in future.
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.shagainst 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: