Proposal for meta-generator #17

pwittrock · 2017-10-30T21:04:00Z

This is a proposal in the form of what would be the user facing documentation for a meta code generation tool.

tamalsaha · 2017-10-30T21:22:49Z

Can we relax the assumption that everything will be inside the pkg package? We like to define things without pkg. For example: https://github.com/appscode/voyager

pwittrock · 2017-10-30T21:29:04Z

@tamalsaha updated

pwittrock · 2017-10-31T00:11:52Z

cc @erictune

tamalsaha · 2017-10-31T10:42:20Z

Thanks @pwittrock ! LGTM!

jennybuckley · 2017-10-31T18:04:01Z

docs/proposals/kubegen.md

+patterns.
+
+- **group pattern**: ^[a-z]+$
+- **version pattern**: ^v\\d+(alpha\\d+|beta\\d+)*$


just wondering, since i haven't seen it in use, would something like "v1alpha1alpha1alpha1alpha1" be accepted as a version?

hm, good point, I don't think that is ok. Should probably be ^v\\d+(alpha\\d+|beta\\d+)?$

Why do we restrict the version pattern here?

Same for the group pattern.

I think it could be useful to restrict, since some generators assume things about how the version will be formatted, like this one

@jennybuckley +1

I have found the code generators and apimachinery makes assumptions about the pattern and break unexpectedly and with obscure errors when they are not followed.

It must be a valid Go identifier. But other than that? Restricting it to alpha/beta is definitely not necessary. If we require that anywhere, we should fix it.

pwittrock · 2017-10-31T23:21:22Z

@stts what is the typical process to get lazy consensus in this repo?

sttts · 2017-11-01T10:04:41Z

docs/proposals/kubegen.md

+- **version pattern**: ^v\\d+(alpha\\d+|beta\\d+)*$
+
+By default, kubegen will run code generators for both external types defined under `pkg/apis/<group>/<version>` and
+internal types defined under `pkg/apis/<group>`.  The location kubegen searches can be overridden.


We have to support the pattern established by k8s.io/api, i.e. that the external APIs are in a different repo. I understand both location can be overridden? locations

added a section about this in the FAQ at the end

sttts · 2017-11-01T10:05:59Z

docs/proposals/kubegen.md

+
+kubegen will prepend all generated files with copyright and license comments.
+
+By default, the kubegen binary runs all* of the Kubernetes code


what is all? The list varies depending on external-type-only projects (usually CRDs of k8s.io/api like repos) and user-provider APIServers.

the list is directly below this statement. crd use cases can manually specify the generators they need

Don't like to specify the generators manually. We have these two pretty well defined use-cases: external types only, internal+external. They should just work out of the box. If things are not needed (e.g. protobuf), this should be auto-detected by the lack of tags.

I think you are right. We may need some break glass in the future, but best not to start with it.

Are there existing tags we should us?

Does this allow us to disable things like deepcopy and conversiongen?

sttts · 2017-11-01T10:06:17Z

docs/proposals/kubegen.md

+- informer-gen
+- lister-gen
+- openapi-gen
+- set-gen


set-gen is independent. It's not run by API groups.

sttts · 2017-11-01T10:35:23Z

docs/proposals/kubegen.md

+The license can be overridden from the LICENSE file using the `--license` flag
+
+```sh
+kubegen --license "Apache 2.0"


How much do we want to deviate from the gengo flag go-header-file. I don't see much additional value of these fine-grained flags.

this provides a simpler bootstrapping experience for new projects by removing an extra step.

sttts · 2017-11-01T10:35:40Z

docs/proposals/kubegen.md

+The license file can be overridden from the LICENSE file using the `--license-file` flag
+
+```sh
+kubegen --license-file "boilerplate.go.txt"


can we re-use the gengo flag name --go-header-file?

we could, but I find this to be more descriptive and that it models the intent better. the go-header-file is in practice just the copyright and license. I would expect most new repos / projects to use the LICENSE file in the project root if it exists. this flag name more closely matches the default.

Took a moment to understand that you want something else. The "boilerplate.go.txt" example flag is wrong.

There is no reason not support both. But I like the idea to use the LICENSE file by default.

How about we start with just --license-file with the description saying it specifies boilerplate.go.txt. Having 2 flags for the same thing is a bit confusing.

sttts · 2017-11-01T10:39:22Z

docs/proposals/kubegen.md

+
+Generating code using only a specific set of generators can be performed using the
+`--generator` flag.  This flag may be provided multiple times to run multiple
+generators.


it's a bit odd that the first --generator arg replaces the "all generators" list with a single generator. Then the second appearance appends the generator. IMO a comma separated --generators is more natural.

This is a common pattern for cobra commands and I believe is preferred. Comma separated lists are hard when there can be commas in the lists.

sttts · 2017-11-01T10:40:13Z

docs/proposals/kubegen.md

+directories.
+
+```sh
+kubegen --apis-dir notpkg/apis --apis-dir pkg/notapis


are these internal api types or external?

either. It is the directory containing the groups.

For some generator it makes a difference. I don't think we should use the same arg. It's a fundamental concept of our apis and our generators that the user easily understands. IMO we should have:

kubegen --apis-dir ... --internal-apis-dir ...

I like this. We will just need to be careful that the default values correctly detect whether internal or external types should be generated. e.g. --internal-apis-dir defaults to pkg/apis, and if there are no internal types defined, it should just skip generating internal types.

sttts · 2017-11-01T10:41:17Z

docs/proposals/kubegen.md

+
+- run all code generators against discovered APIs
+  - search for API group versions under notpkg/apis and pkg/notapis instead of pkg/apis  
+


What about the output-package?

Do we pass through any gengo args?

For reference, these are the gengo flags:

fs.StringSliceVarP(&g.InputDirs, "input-dirs", "i", g.InputDirs, "Comma-separated list of import paths to get input types from.") fs.StringVarP(&g.OutputBase, "output-base", "o", g.OutputBase, "Output base; defaults to $GOPATH/src/ or ./ if $GOPATH is not set.") fs.StringVarP(&g.OutputPackagePath, "output-package", "p", g.OutputPackagePath, "Base package path.") fs.StringVarP(&g.OutputFileBaseName, "output-file-base", "O", g.OutputFileBaseName, "Base name (without .go suffix) for output files.") fs.StringVarP(&g.GoHeaderFilePath, "go-header-file", "h", g.GoHeaderFilePath, "File containing boilerplate header text. The string YEAR will be replaced with the current 4-digit year.") fs.BoolVar(&g.VerifyOnly, "verify-only", g.VerifyOnly, "If true, only verify existing output, do not write anything.") fs.StringVar(&g.GeneratedBuildTag, "build-tag", g.GeneratedBuildTag, "A Go build tag to use to identify files generated by this command. Should be unique.")

There might be generator-specific flags we have to pass through as well. We should have a way to do this and/or look through the existing gen-specific flags.

Agreed, though I would prefer to initially provide the simplest interface that works, and then add this additional complexity as we discover it is needed.

Lets start with defaulting the output package and add it when we discover it is necessary.

Actually, it's only --verify-only I care about right now from the generic flags. But there are more converter specific flags. Just saying that we should be prepared to offer converter specific ones, e.g. --conversion-gen-exceptions-file, --client-gen-clientset-name.

SGTM. I am proposing exposing --verify-only as --dry-run since it is a much more common flag name and has the same meaning. Thoughts?

sttts · 2017-11-01T10:44:21Z

what is the typical process to get lazy consensus in this repo?

I don't think there is a policy for that. I guess in kubernetes/community the proposal process is written down somewhere.

pwittrock · 2017-11-01T22:42:33Z

Moved to kubernetes/community#1315

pwittrock · 2017-11-01T22:44:08Z

Thanks for all your comments @sttts I tried to respond or address all of them.

The flag name changes I am suggesting are trying to make it as simple and understandable for folks that don't already have the background and history of the code generators. e.g. the history of boilerplate.go.txt.

sttts · 2017-11-02T06:41:58Z

Left a couple of more comments. WIll switch to the new community PR now.

pwittrock · 2017-11-02T16:25:09Z

@sttts Addressed comments in the community repo.

Proposal for meta-generator

aa77b71

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 30, 2017

pwittrock mentioned this pull request Oct 30, 2017

code-generator: create stable and more extensible cli interface for top-level scripts – "kube-gen" kubernetes/kubernetes#53524

Closed

Support looking for APIs outside pkg/apis

7d4bd09

pwittrock requested review from sttts and caesarxuchao October 31, 2017 14:10

pwittrock assigned sttts and caesarxuchao Oct 31, 2017

jennybuckley reviewed Oct 31, 2017

View reviewed changes

sttts reviewed Nov 1, 2017

View reviewed changes

address pr comments

b7b20ee

pwittrock closed this Nov 1, 2017


		kubegen will prepend all generated files with copyright and license comments.

		By default, the kubegen binary runs all* of the Kubernetes code


		- run all code generators against discovered APIs
		- search for API group versions under notpkg/apis and pkg/notapis instead of pkg/apis

Proposal for meta-generator #17

Proposal for meta-generator #17

Conversation

pwittrock commented Oct 30, 2017 • edited Loading

tamalsaha commented Oct 30, 2017

pwittrock commented Oct 30, 2017

pwittrock commented Oct 31, 2017

tamalsaha commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Nov 1, 2017

pwittrock commented Nov 1, 2017

pwittrock commented Nov 1, 2017

sttts commented Nov 2, 2017

pwittrock commented Nov 2, 2017

pwittrock commented Oct 30, 2017 •

edited

Loading

pwittrock Nov 2, 2017 •

edited

Loading