-
Notifications
You must be signed in to change notification settings - Fork 39.6k
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-generator: rewrite hack/update-codecs.sh into reusable generate-{internal,}-groups.sh #52186
code-generator: rewrite hack/update-codecs.sh into reusable generate-{internal,}-groups.sh #52186
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sttts Assign the PR to them by writing No associated issue. Update pull-request body to add a reference to an issue, or get approval with The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/cc @mrIncompetent |
@sttts: GitHub didn't allow me to request PR reviews from the following users: mrIncompetent. Note that only kubernetes members can review this PR, and authors cannot review their own PRs. In response to this:
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. |
This is wonderful! Very much needed! |
86d2bc7
to
ab37198
Compare
@eparis can you help review the bash? |
/cc @colemickens @nikhita – could you take a look at this? It is especially meant to make the life of CRD users easier. |
/cc @ericchiang @scottrigby @kubernetes/sig-apps-misc – review and feedback is welcome |
IFS=: read G Vs <<<"${GVs}" | ||
|
||
# enumerate versions | ||
for V in ${Vs//,/ } do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a missing ;
after for V in ${Vs//,/ }
here. I get the following when running on OS X:
Adding it resolves the following:
/Users/james/go/src/k8s.io/kubernetes/vendor/k8s.io/code-generator/hack/lib/codegen.sh:98: parse error near 'done'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, missing ;
It'd be great if this library could also expose the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sttts
Awesome work!
A shared docker container like k8s.io/code-generator:1.8
would be very useful too!
|
||
for gen in defaulter-gen conversion-gen client-gen lister-gen informer-gen deepcopy-gen conversion-gen; do | ||
echo "Building ${gen}" | ||
go install ${CODEGEN_PKG}/cmd/${gen} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This incurs side effect for source-ing this lib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true. We should do this only on-demand and only once.
} | ||
|
||
function generate-groups() { | ||
local BASE="$1" # the base packages are resolved against (empty or the vendor/ directory ${OUTPUT_PKG} is in) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to confess these vars are difficult to understand. Can you give some specific examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simplified this. BASE is usually not needed, only in the kube case where we have staging/ directories.
@sttts |
e2753b1
to
e56003d
Compare
@ericchiang @munnerz addressed your comments. Moreover, I have switch from the codegen.sh library approach to two executable scripts in From my side this is complete now to go into master and the be cherry-picked into 1.8. A possible Golang rewrite has to wait for 1.9. |
ce13dc6
to
590d493
Compare
lgtm if tests pass |
590d493
to
189dfba
Compare
…internal,}-groups.sh scripts
189dfba
to
96b5961
Compare
/retest |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue (batch tested with PRs 53317, 52186). If you want to cherry-pick this change to another branch, please follow the instructions here. |
@sttts: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
…upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #52186 ```release-note Add generate-groups.sh and generate-internal-groups.sh to k8s.io/code-generator to easily run generators against CRD or User API Server types. ```
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. sample-controller: add example CRD controller **What this PR does / why we need it**: Adds a sample-controller example repository fixes #52752 **Special notes for your reviewer**: This is currently based on the sttts:sttts-codegen-scripts branch and should not be merged until that is (ref #52186) **Release note**: ``` Add sample-controller repository ``` /cc @sttts @nikhita @colemickens
@pwittrock after seeing your community meeting slides with |
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. sample-controller: add example CRD controller **What this PR does / why we need it**: Adds a sample-controller example repository fixes #52752 **Special notes for your reviewer**: This is currently based on the sttts:sttts-codegen-scripts branch and should not be merged until that is (ref kubernetes/kubernetes#52186) **Release note**: ``` Add sample-controller repository ``` /cc @sttts @nikhita @colemickens Kubernetes-commit: 9a7800f7d2efb88b397674672ac56f898826cf7c
Generating everything for groups inside of an apiserver (with internal types) becomes:
generate-internal-groups.sh all "$(dirname ${BASH_SOURCE})/../../.." k8s.io/sample-apiserver/pkg/client k8s.io/sample-apiserver/pkg/apis k8s.io/sample-apiserver/pkg/apis wardle:v1alpha1
Generating everything for a CRD (versioned types) becomes:
generate-groups.sh all "$(dirname ${BASH_SOURCE})/../../.." k8s.io/sample-apiserver/pkg/client k8s.io/sample-apiserver/pkg/apis wardle:v1alpha1
This should cover the 90% percent use-case. For the other 10% this can be forked and adapted as needed.
Furthermore, we can put this into a Docker container. Then code-generator consumers can then do:
$ docker run -v $GOPATH:/go k8s.io/code-generator:1.8 generate-group.sh github.com/foo/bar example:v1
This is possibly only the first step towards a
code-generator
binary. For the later deeper generator changes are necessary (e.g. #53202) and hence the later is only feasible in 1.9. This PR here in contrast, we can cherry-pick to 1.8.Fixes #48714.