code-generator/client-gen: decouple core group from package name 'api' #125162

sttts · 2024-05-28T09:06:23Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

client-gen didn't work with api/v1-like directory structures. This PR removes special casing of the api package name, and move that logic into the // +groupName= marker.

This PR also adds an example for a single API group to ensure the generated code is correct. Compare how client-gen is launched to support this single group:

kube::codegen::gen_client \
    --with-watch \
    --with-applyconfig \
    --output-dir "${SCRIPT_ROOT}/single" \
    --output-pkg "${THIS_PKG}/single" \
    --boilerplate "${SCRIPT_ROOT}/hack/boilerplate.go.txt" \
    --one-input-api "api" \
    "${SCRIPT_ROOT}/single"

Which issue(s) this PR fixes:

Fixes kubernetes/code-generator#167

Special notes for your reviewer:

Note that there are no changes to generated client-go code for the legacy core group.

Also note that the generated client for the "single" example uses config.APIPath = "/apis", not /api as the legacy core group client (this would have been the case before this PR).

Does this PR introduce a user-facing change?

Fix code-generator client-gen to work with `api/v1`-like package structure.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-triage-robot · 2024-05-28T11:06:00Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

cici37 · 2024-05-28T20:26:12Z

/triage accepted

pkg/apis/core/doc.go

thockin · 2024-05-30T19:53:23Z

api/api-rules/codegen_violation_exceptions.list

@@ -7,6 +7,7 @@ API rule violation: names_match,k8s.io/apimachinery/pkg/apis/meta/v1,StatusCause
 API rule violation: names_match,k8s.io/apimachinery/pkg/apis/meta/v1,Time,Time
 API rule violation: names_match,k8s.io/apimachinery/pkg/runtime,Unknown,ContentEncoding
 API rule violation: names_match,k8s.io/apimachinery/pkg/runtime,Unknown,ContentType
+API rule violation: names_match,k8s.io/code-generator/examples/apiserver/apis/core/v1,TestTypeStatus,Blah


I can't tell if this is intentional (ie. the lack of a JSON name matters to the test) or just because we are too lazy to fix these?

probably lazy and good cargo culting of the broken example. Let me check.

staging/src/k8s.io/api/core/v1/doc.go

staging/src/k8s.io/code-generator/cmd/client-gen/generators/generator_for_group.go

mdbooth · 2024-06-25T16:03:27Z

I can confirm that this PR allows me to generate applyconfiguration and clientset in https://github.com/kubernetes-sigs/cluster-api-provider-openstack.

The gen_client invocation I tested is:

kube::codegen::gen_client \
    --with-watch \
    --with-applyconfig \
    --output-dir "${PROJECT_ROOT}/client" \
    --output-pkg sigs.k8s.io/cluster-api-provider-openstack/client \
    --versioned-name clientset \
    --boilerplate "${SCRIPT_ROOT}/boilerplate.go.txt" \
    --one-input-api "api" \
    "${PROJECT_ROOT}"

That's slightly different to the example in the description as the input argument is the project root rather than the output directory.

thockin

Thanks!

/lgtm
/approve

thockin · 2024-06-25T22:19:20Z

staging/src/k8s.io/code-generator/cmd/client-gen/generators/generator_for_group.go

@@ -71,31 +71,25 @@ func (g *genGroup) Imports(c *generator.Context) (imports []string) {
 func (g *genGroup) GenerateType(c *generator.Context, t *types.Type, w io.Writer) error {
 	sw := generator.NewSnippetWriter(w, c, "$", "$")

-	apiPath := func(group string) string {


I had to backtrack to find where group is being set to "core" - Group.NonEmpty() says:

func (g Group) NonEmpty() string { if g == "api" { return "core" } return string(g) }

So where does "api" come from? I guess that comes from the filesystem layout of the legacy group.

Do you think it would make sense to move legacy code or somehow "normalize" it so these weird special cases go away?

That "api" is now gone, replaced with "". Core is still hardcoded. But that's just a symbol for the go types.

k8s-ci-robot · 2024-06-25T22:19:36Z

LGTM label has been added.

Git tree hash: d4fb258aa250e1447a6fe1ffcbbec486894bfb94

k8s-ci-robot · 2024-06-25T22:19:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts, thockin

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:

~~api/OWNERS~~ [thockin]
~~pkg/apis/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]
~~staging/src/k8s.io/code-generator/OWNERS~~ [sttts,thockin]

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

pacoxu · 2024-06-26T03:58:14Z

/retest

pacoxu · 2024-06-26T05:01:06Z

/hold
for ci failure
Feel free to unhold once the CI is green.

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

mdbooth · 2024-06-27T15:05:14Z

I've retested this locally. I am still able to use this to generate a clientset in cluster-api-provider-openstack.

/lgtm

k8s-ci-robot · 2024-06-27T15:05:21Z

LGTM label has been added.

Git tree hash: b25e6e60cf126c99c8677963d1e740dc1543ad65

shaneutt · 2024-06-27T15:12:52Z

Just wanted to throw a thank you to everyone who worked on this, it is appreciated! 🫡

thockin · 2024-06-27T15:50:57Z

@sttts was this last push just a rebase? I can't discern what relevant changes are in the diff?

/lgtm
/hold JIC I missed something?

sttts · 2024-06-27T22:07:18Z

Yes, rebase and codegen, some go docs changed.

sttts · 2024-06-27T22:07:37Z

/hold cancel

sttts assigned thockin May 28, 2024

k8s-ci-robot requested review from caesarxuchao and cici37 May 28, 2024 09:06

sttts mentioned this pull request May 28, 2024

client-gen generates empty "core" group clientsets when "api" package directory is in use for input APIs kubernetes/code-generator#167

Closed

sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 28, 2024

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label May 28, 2024

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 28, 2024

sttts force-pushed the sttts-code-generator-core-group branch from c27b22b to 4186e8c Compare May 29, 2024 20:41

thockin reviewed May 30, 2024

View reviewed changes

vladikkuzn mentioned this pull request Jun 2, 2024

Kjobctl bootstrap kubernetes-sigs/kueue#2307

Merged

2 tasks

sttts force-pushed the sttts-code-generator-core-group branch from e3f6314 to 2edb86b Compare June 4, 2024 18:22

thockin reviewed Jun 25, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2024

sttts added 5 commits June 27, 2024 15:16

code-generator/client-gen: add example with core group

7e52d34

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

code-generator/client-gen: decouple core group from package name 'api'

6b2f779

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

code-generator/client-gen: add example with single package api/v1

be2cf7e

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

code-generator/examples: fix json tags

676d989

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

make codegen

ac3b764

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

sttts force-pushed the sttts-code-generator-core-group branch from 2edb86b to ac3b764 Compare June 27, 2024 13:33

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2024

k8s-ci-robot requested a review from thockin June 27, 2024 13:33

k8s-ci-robot assigned mdbooth Jun 27, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 27, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2024

k8s-ci-robot merged commit 2c6daa4 into kubernetes:master Jun 27, 2024
14 checks passed

k8s-ci-robot added this to the v1.31 milestone Jun 27, 2024

twz123 mentioned this pull request Jul 4, 2024

✨ Make the anonymous field of the groupName marker optional kubernetes-sigs/controller-tools#1000

Merged

kerthcet mentioned this pull request Jul 17, 2024

Remove core folder InftyAI/llmaz#24

Open

jcaamano mentioned this pull request Aug 21, 2024

client-gen API not generated correctly metallb/frr-k8s#185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

code-generator/client-gen: decouple core group from package name 'api' #125162

code-generator/client-gen: decouple core group from package name 'api' #125162

sttts commented May 28, 2024 •

edited

Loading

k8s-triage-robot commented May 28, 2024

cici37 commented May 28, 2024

thockin May 30, 2024

sttts Jun 4, 2024

sttts Jun 4, 2024

mdbooth commented Jun 25, 2024

thockin left a comment

thockin Jun 25, 2024

sttts Jun 27, 2024 •

edited

Loading

k8s-ci-robot commented Jun 25, 2024

k8s-ci-robot commented Jun 25, 2024

pacoxu commented Jun 26, 2024

pacoxu commented Jun 26, 2024

mdbooth commented Jun 27, 2024

k8s-ci-robot commented Jun 27, 2024

shaneutt commented Jun 27, 2024

thockin commented Jun 27, 2024

sttts commented Jun 27, 2024

sttts commented Jun 27, 2024

code-generator/client-gen: decouple core group from package name 'api' #125162

code-generator/client-gen: decouple core group from package name 'api' #125162

Conversation

sttts commented May 28, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-triage-robot commented May 28, 2024

cici37 commented May 28, 2024

thockin May 30, 2024

Choose a reason for hiding this comment

sttts Jun 4, 2024

Choose a reason for hiding this comment

sttts Jun 4, 2024

Choose a reason for hiding this comment

mdbooth commented Jun 25, 2024

thockin left a comment

Choose a reason for hiding this comment

thockin Jun 25, 2024

Choose a reason for hiding this comment

sttts Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 25, 2024

k8s-ci-robot commented Jun 25, 2024

pacoxu commented Jun 26, 2024

pacoxu commented Jun 26, 2024

mdbooth commented Jun 27, 2024

k8s-ci-robot commented Jun 27, 2024

shaneutt commented Jun 27, 2024

thockin commented Jun 27, 2024

sttts commented Jun 27, 2024

sttts commented Jun 27, 2024

sttts commented May 28, 2024 •

edited

Loading

sttts Jun 27, 2024 •

edited

Loading