Skip to content

✨ Add things for kcp-dev/client-go#111

Merged
kcp-ci-bot merged 4 commits intokcp-dev:kcp-1.32.3from
ntnn:code-gen-fixes
Jun 17, 2025
Merged

✨ Add things for kcp-dev/client-go#111
kcp-ci-bot merged 4 commits intokcp-dev:kcp-1.32.3from
ntnn:code-gen-fixes

Conversation

@ntnn
Copy link
Copy Markdown
Member

@ntnn ntnn commented Jun 17, 2025 •
edited
Loading

Summary

Adds things to genearte kcp-dev/client-go with code-generator/v3

Related issue(s)

Fixes #

Release Notes

Add --exclude-group-versions to cluster::codegen::gen_client
Base client package name off of import path instead of using 'clientset'
Explicitly check verbs list and watch on types

@ntnn
Check list/watch verbs on types
34af3cd 
Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
@kcp-ci-bot kcp-ci-bot added dco-signoff: yes Indicates the PR's author has signed the DCO. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 17, 2025
@ntnn ntnn mentioned this pull request Jun 17, 2025
Merged
@ntnn
Copy link
Copy Markdown
Member Author

ntnn commented Jun 17, 2025

Add --exclude-packages to cluster::codegen::gen_client

Required as imagepolicy/v1alpha1 is in k8s.io/api but not in k8s.io/client-go.
I looked into basing package exclusion off of KUBE_NONSERVER_GROUP_VERSIONS but overall that was more work than just listing the one package we do not need.
https://github.com/kubernetes/kubernetes/blob/3e39d1074fc717a883aaf57b966dd7a06dfca2ec/hack/lib/init.sh#L130

Base client package name off of import path instead of using 'clientset'

Because github.com/kcp-dev/client-go/kubernetes should continue to be named kubernetes and not clientset

ntnn
ntnn commented Jun 17, 2025
View reviewed changes
Comment thread cmd/cluster-client-gen/generators/client_generator.go
func targetForClientset(args *args.Args, clientsetDir, clientsetPkg string, groupGoNames map[clientgentypes.GroupVersion]string, boilerplate []byte) generator.Target {
return &generator.SimpleTarget{
PkgName: "clientset",
PkgName: path.Base(clientsetPkg),
Copy link
Copy Markdown
Member Author

@ntnn ntnn Jun 17, 2025

Choose a reason for hiding this comment

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

Could also make this an input, e.g. clientsetPkgName but imho thats just another possible error source and path.Base should always return the correct value.

Copy link
Copy Markdown
Contributor

@xrstf xrstf Jun 17, 2025

Choose a reason for hiding this comment

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

Just make sure this works both in the code-generator's examples and for kcp itself. Both have widely different directory structures, so assumptions are difficult to make here 😁

Copy link
Copy Markdown
Member Author

@ntnn ntnn Jun 17, 2025

Choose a reason for hiding this comment

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

I had only noticed this because the tests in kcp were complaining about the mismatch :D

@ntnn ntnn requested a review from xrstf June 17, 2025 13:04
xrstf
xrstf reviewed Jun 17, 2025
View reviewed changes
Comment thread cluster_codegen.sh Outdated
# An optional list of comma separated plural exception definitions in Type:PluralizedType form.
#
# --exclude-packages <string = "">
# An optional list of comma separate group verions to exclude from
Copy link
Copy Markdown
Contributor

@xrstf xrstf Jun 17, 2025

Choose a reason for hiding this comment

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

versions

@ntnn ntnn force-pushed the code-gen-fixes branch from 0ee7a3c to fd3c22e Compare June 17, 2025 14:58
ntnn added 3 commits June 17, 2025 17:03
@ntnn
Add --exclude-group-versions
82f9e9b 
Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
@ntnn
Set package name based off of clientsetPkg
33f747c 
Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
@ntnn
codegen
88add97 
Signed-off-by: Nelo-T. Wallus <n.wallus@sap.com>
@ntnn ntnn force-pushed the code-gen-fixes branch from fd3c22e to 88add97 Compare June 17, 2025 15:03
@ntnn
Copy link
Copy Markdown
Member Author

ntnn commented Jun 17, 2025

I also looked into generating the ...Resource and ...Kind variables like in v2: ntnn@3aa7e05
I added those manually in client-go: https://github.com/kcp-dev/client-go/pull/46/files#diff-f1ca00fd1198202f5b6f4fe8422584a003e2ba1d1f9f41cbbe3526a9f355ecb8

However that fails on the group - it doesn't look like the group is readily available and v2 built it while parsing the source.
It's only private and not required in every package, so imho it's fine to just add it in the _expansion.go files where necessary.
The alternative would be construing the group based off of the package path.

xrstf
xrstf approved these changes Jun 17, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@xrstf xrstf left a comment

Choose a reason for hiding this comment

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

/approve

@kcp-ci-bot kcp-ci-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2025
@kcp-ci-bot
Copy link
Copy Markdown
Contributor

kcp-ci-bot commented Jun 17, 2025

LGTM label has been added.

DetailsGit tree hash: e249095f3d4ba7189fac29cdc3b9fb386a744736

@kcp-ci-bot
Copy link
Copy Markdown
Contributor

kcp-ci-bot commented Jun 17, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xrstf

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

The pull request process is described here

Details 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

@kcp-ci-bot kcp-ci-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2025
@kcp-ci-bot kcp-ci-bot merged commit 126a1bc into kcp-dev:kcp-1.32.3 Jun 17, 2025
10 checks passed
@ntnn ntnn deleted the code-gen-fixes branch June 17, 2025 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xrstf xrstf xrstf approved these changes

Assignees

@xrstf xrstf

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has signed the DCO. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ntnn @kcp-ci-bot @xrstf