✨ v3: rewrite to work the same as kubernetes code-generator #107
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kcp-ci-bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
kcp-ci-bot
added
dco-signoff: yes
xrstf
force-pushed
the
rewrite-to-upstream
branch
from
aa56fdf to
0663593
Compare
|
PR needs rebase. 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. |
kcp-ci-bot
added
the
needs-rebase
xrstf
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test all |
…old code On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
This * makes the generated-by comment clearer to distinguish from k8s' tools * allows our binaries to co-exist in the same directory as the k8s binaries On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
kcp uses an entirely different concept of placing the packages. Whereas the examples in this repo follow a / <singlecluster> <-- all single-cluster code / <base outputdir> +-- clientset/versioned/ <-- cluster-aware clientset +-- informers/ +-- listers/ structure, but in kcp it's / <singlecluster> +-- clientset/ | +-- versioned/ <-- single-cluster clientset | +-- cluster/ <-- cluster-ware clientset +-- listers +-- informers Placing the cluster clientset right into the singlecluster clientset, but keeping the listers and informers standalone (since there are no single-cluster listers and informers in the kcp SDK). Making both structures possible would either require a bunch of magic, or some explicit, IMHO easier to understand CLI flags to control exactly what you want to have happen. I have sacrified to a degree the concept of "naming" a clientset and the "versioned" subdirectory, also because in kcp the cluster-aware clientset doesn't itself contain a clientset/versioned/ subdirectory. On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
It seems even though we did generate the applyconfigurations package already, we never told the kube-client-gen about it and so it never generated the ...WithApply variant with Apply and ApplyStatus functions. That's what is causing this diff to be larger than expected. On-behalf-of: @SAP [email protected]
xrstf
force-pushed
the
rewrite-to-upstream
branch
from
99b3bd9 to
c9daefe
Compare
|
/test all |
xrstf
force-pushed
the
rewrite-to-upstream
branch
from
c9daefe to
3e1589b
Compare
Since most of the new code is copied straight from Kubernetes, I felt uncomfortable to relicense it as Copyright KCP, so instead I chose to apply a dual-copyright. Ideally the KCP one should have been after the Kubernetes line, but that is not possible with verify_boilerplate.py. Since the examples are kcp-only, they got their own boilerplates. It felt weird to have the generated code be dual-copyrighted. On-behalf-of: @SAP [email protected]
xrstf
force-pushed
the
rewrite-to-upstream
branch
from
8586dc2 to
a20e483
Compare
|
/test all |
disabling prealloc linter because its documentation states: > IMPORTANT: we don't recommend using this linter before doing performance profiling. > For most programs usage of prealloc will be a premature optimization. On-behalf-of: @SAP [email protected]
|
/test all |
kcp-ci-bot
removed
the
do-not-merge/work-in-progress
On-behalf-of: @SAP [email protected]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This represents a major overhaul of the entire kcp code-generator. So much so that I think this warrants a jump to v3.
The main idea here is to align ourselves with the architecture and ways of thinking of the Kubernetes code-generator. Up until now our own code-generator was arguably simpler, but also more limited, making it harder to adopt new upstream changes.
Overview
From 10,000ft this PR
cluster-...to make the distinction from upstream clear and to allow users to download all generators (kube and kcp) into the same directory, if they wanted to,cluster_codegen.sh, our new equivalent of Kube'skube_codegen.sh,kube_codegen.sh.Things of Note
Package naming
It turned out that kcp and the code-generator's examples use a fundamentally different directory structure (see the related commit in this PR). I can see value in both styles and I think we should support both.
For this reason I decided to take out some of the implicit Kubernetes logic ("magic" some might call it) and instead allow the user full control over the packages and directories. This means instead of
--listers-namewhich might be appended to some other package name, users simply provide the full package and directory names for where they want the listers to be generated. There is still some defaulting, but only in thecluster_codegen.sh. If users choose to use the generator binaries manually, they would have to provide more explicit packages and directories.Package Aliases
One downside of Kube's approach to generating code is that package aliases are mostly automatically generated, which yields ... sometimes unintuitive results. It would be possible to generate more "human-friendly" aliases, but since they are only of use and note within the generated code itself, I chose to accept my fate and the many super ugly aliases.
Fun fact: Upstream tries to get rid of hardcoded imports in the generator as much as possible, yet I would have tried the exact opposite and generate even more hardcoded imports. I think this stems from the fundamentally different view on code-generation: upstream seeing it as creating an AST and then rendering it as Go, I would have seen it more as "a Go file with placeholders". Again, I think both are valid, but since upstream so much favors dynamic imports, I also used them in many places, leading to even more ugly aliases 😁
Blind Spots
There is a good chance that there might exist some combination of API types and generator flags that produce incomplete output. I have mostly focussed on getting the examples and kcp to work, but if someone manages to introduce internal versions into CRDs, they might be SOL because the code-generator cannot handle these at the moment.
Protobuf
I kept out the prefersProtobuf part for now and am planning to introduce that in a standalone PR, purely in an attempt to keep this PR smaller.
Upgrading
I would strongly recommend users upgrading to v3 to use the
cluster_codegen.shfrom now on, just like upstream recommendskube_codegen.sh.Interfaces and structs stay compatible and the package structure is too, so code using the generated code should not require adjustments.
Related issue(s)
Fixes #18
Release Notes