Skip to content
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

Merged
merged 5 commits into from
Oct 4, 2017

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Sep 8, 2017

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.

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.

Fixes #48714.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 8, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sttts
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 8, 2017
@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 8, 2017
@sttts sttts assigned caesarxuchao and deads2k and unassigned smarterclayton Sep 8, 2017
@sttts sttts added kind/enhancement kind/documentation Categorizes issue or PR as related to documentation. labels Sep 8, 2017
@sttts
Copy link
Contributor Author

sttts commented Sep 8, 2017

/cc @mrIncompetent

@k8s-ci-robot
Copy link
Contributor

@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:

/cc @mrIncompetent

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.

@mrIncompetent
Copy link
Contributor

This is wonderful! Very much needed!

@deads2k
Copy link
Contributor

deads2k commented Sep 12, 2017

@eparis can you help review the bash?

@sttts
Copy link
Contributor Author

sttts commented Sep 19, 2017

/cc @colemickens @nikhita – could you take a look at this? It is especially meant to make the life of CRD users easier.

@sttts
Copy link
Contributor Author

sttts commented Sep 19, 2017

/cc @ericchiang @scottrigby @kubernetes/sig-apps-misc – review and feedback is welcome

@k8s-ci-robot k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Sep 19, 2017
IFS=: read G Vs <<<"${GVs}"

# enumerate versions
for V in ${Vs//,/ } do
Copy link
Member

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'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, missing ;

@munnerz
Copy link
Member

munnerz commented Sep 19, 2017

It'd be great if this library could also expose the --verify flag of it's underlying generators. I've found this really useful in CI, to ensure that generated files are up to date in the repo.

Copy link
Contributor

@hongchaodeng hongchaodeng left a 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}
Copy link
Contributor

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.

Copy link
Contributor Author

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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@hongchaodeng
Copy link
Contributor

@sttts
Can you assign to me? I can help test the script out.

@sttts sttts changed the title code-generator: rewrite hack/update-codecs.sh into a reusable library code-generator: rewrite hack/update-codecs.sh into reusable generate-{internal,}-groups.sh Sep 28, 2017
@sttts
Copy link
Contributor Author

sttts commented Sep 28, 2017

@ericchiang @munnerz addressed your comments. Moreover, I have switch from the codegen.sh library approach to two executable scripts in k8s.io/code-generator. This is more in-line with the future code-generator binary, if we decide to rewrite the shell scripts to Go.

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.

@sttts sttts force-pushed the sttts-codegen-scripts branch 2 times, most recently from ce13dc6 to 590d493 Compare September 28, 2017 14:11
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2017
@sttts sttts added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Sep 28, 2017
@hongchaodeng
Copy link
Contributor

lgtm if tests pass

@sttts
Copy link
Contributor Author

sttts commented Oct 4, 2017

/retest

@sttts sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

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.

@k8s-github-robot k8s-github-robot merged commit 1357cef into kubernetes:master Oct 4, 2017
@k8s-ci-robot
Copy link
Contributor

@sttts: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce ab37198 link /test pull-kubernetes-federation-e2e-gce
pull-kubernetes-cross e2753b1 link /test pull-kubernetes-cross
pull-kubernetes-e2e-gce-bazel 96b5961 link /test pull-kubernetes-e2e-gce-bazel
pull-kubernetes-e2e-gce-etcd3 96b5961 link /test pull-kubernetes-e2e-gce-etcd3

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.

k8s-github-robot pushed a commit that referenced this pull request Oct 10, 2017
…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.
```
k8s-github-robot pushed a commit that referenced this pull request Oct 19, 2017
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
@sttts
Copy link
Contributor Author

sttts commented Oct 23, 2017

@pwittrock after seeing your community meeting slides with Support for bootstrapping CRD go clients, go controllers, ref docs?, would be great to get your input on these kind of changes.

sttts pushed a commit to sttts/sample-controller that referenced this pull request Oct 26, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.