Default-gen changes for moving API types to k8s.io/api #54

caesarxuchao · 2017-05-12T21:55:11Z

Currently, the k8s:defaulter-gen tag is in the k8s.io/kubernetes/pkg/apis/<group>/<version>/doc.go, co-located with the input (API types) and the output (default func) of default-gen.

After kubernetes/kubernetes#44065 is implemented, the input/output/tag are not going to be in the same package:

The input, the API types will be moved to k8s.io/api;
The output, the generated default funcs, will stay in k8s.io/kubernetes, because only apiserver should ever need them; the tag (i.e., doc.go) will also stay in k8s.io/kubernetes.

This PR adds the k8s:defaulter-gen-input tag, so that the API types can be supplied from a different package.

@lavalamp @thockin

k8s-reviewable · 2017-05-12T21:55:17Z

This change is

caesarxuchao · 2017-05-12T21:56:03Z

I'll add some tests.

caesarxuchao · 2017-05-12T23:31:07Z

@mbohlool could you help review it? Thanks.

mbohlool · 2017-05-15T23:56:07Z

LGTM, do you want to add some tests before merging this?

caesarxuchao · 2017-05-15T23:56:48Z

Yes, after the other is merged.

caesarxuchao · 2017-05-30T23:18:21Z

The tests PR (#55) is not going anywhere, can we get this merged before code freeze? It's a noop currently until i set up the codegen tags in kubernetes/kubernetes#44784.

lavalamp · 2017-05-30T23:22:48Z

parser/parse.go

@@ -260,6 +260,18 @@ func (b *Builder) AddDirTo(dir string, u *types.Universe) error {
 	return b.findTypesIn(canonicalizeImportPath(b.buildPackages[dir].ImportPath), u)
 }

+// AddDirToAndReturnAddedPath is the same as AddDirTo except that it also
+// returns the path of the added package.
+func (b *Builder) AddDirToAndReturnAddedPath(dir string, u *types.Universe) (string, error) {


Instead of doing this, add a method that canonicalizes a path and returns a *Package object.

caesarxuchao · 2017-05-31T00:17:37Z

parser/parse.go

@@ -260,6 +260,26 @@ func (b *Builder) AddDirTo(dir string, u *types.Universe) error {
 	return b.findTypesIn(canonicalizeImportPath(b.buildPackages[dir].ImportPath), u)
 }

+func (b *Builder) CanonicalPath(dir string) (string, error) {


Do you mean adding a method like this?

how about DirectoryToPackage(dir string, u *types.Universe) (*types.Package, error)

Returning the package is not going to help me. I need the canonicalizeImportPath(package.ImportPath).

lavalamp · 2017-05-31T00:27:22Z

parser/parse.go

+
+// AddDirToAndReturnAddedPath is the same as AddDirTo except that it also
+// returns the path of the added package.
+func (b *Builder) AddDirToAndReturnAddedPath(dir string, u *types.Universe) (string, error) {


and remove this function

caesarxuchao · 2017-05-31T00:59:53Z

parser/parse.go

@@ -260,6 +260,16 @@ func (b *Builder) AddDirTo(dir string, u *types.Universe) error {
 	return b.findTypesIn(canonicalizeImportPath(b.buildPackages[dir].ImportPath), u)
 }

+// DirectoryToPackage returns the package path given a dir. dir must be already
+// parsed.
+func (b *Builder) DirectoryToPackagePath(dir string) (string, error) {


How about this?

Returning Package is not going to help me, because what I need is canonicalizeImportPath(package.ImportPath).

I actually feel my original code is better. This version makes the generator call context.AddDir and context.DirectoryToPackagePath, while my version only calls context.AddDirAndReturnAddedPkgPath.

I still think my first version is better. I can give the method a better name, maybe AddDirectory (just to be different from AddDir). WDYT?

lavalamp · 2017-05-31T17:30:48Z

examples/defaulter-gen/generators/defaulter.go

+				glog.Fatalf("cannot translate dir %s to package path package %s", inputDir)
+			}
+			var ok bool
+			typesPkg, ok = context.Universe[inputPkgPath]


Isn't this just getting the package? Why do you need its path?

context.Universe[inputPkgPath] returns k8s.io/gengo/types#Package.
context.AddDirTo calls builder to parse the source code, which only knows go/build#Package and go/types#Package.

The translation from directory to canonical path is done in builder, i can add a method to it to export the functionality, and then export this method via context. But I think that is worse than just return the package path via AddDirTo.

caesarxuchao · 2017-05-31T20:15:57Z

Updated. PTAL. Thanks.

lavalamp · 2017-05-31T20:19:01Z

parser/parse.go

-// generator (rather than just at init time. 'dir' must be a single go package.
-// GOPATH, GOROOT, and the location of your go binary (`which go`) will all be
-// searched if dir doesn't literally resolve.
+// Deprecated. Please use AddDirectoryTo.


Please leave the former godoc comment and just append this note :)

lavalamp · 2017-05-31T20:19:15Z

generator/generator.go

-// AddDir adds a Go package to the context. The specified path must be a single
-// go package import path.  GOPATH, GOROOT, and the location of your go binary
-// (`which go`) will all be searched, in the normal Go fashion.
+// Deprecated. Please use AddDirectory.


lavalamp · 2017-05-31T20:30:39Z

LGTM

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 12, 2017

caesarxuchao mentioned this pull request May 18, 2017

Allow conversion-gen to take types that are not in the output path kubernetes/kubernetes#45057

Merged

lavalamp reviewed May 30, 2017

View reviewed changes

caesarxuchao commented May 31, 2017

View reviewed changes

lavalamp reviewed May 31, 2017

View reviewed changes

caesarxuchao force-pushed the fix-defaut-gen branch from 3fcd613 to 17faa00 Compare May 31, 2017 00:55

caesarxuchao commented May 31, 2017

View reviewed changes

sttts mentioned this pull request May 31, 2017

Fix defaulting in staging/ dirs #60

Merged

lavalamp reviewed May 31, 2017

View reviewed changes

caesarxuchao force-pushed the fix-defaut-gen branch 2 times, most recently from 4e6277e to 6bcea36 Compare May 31, 2017 20:15

lavalamp reviewed May 31, 2017

View reviewed changes

fix default-gen

92c280c

caesarxuchao force-pushed the fix-defaut-gen branch from 6bcea36 to d6ea4d5 Compare May 31, 2017 20:25

change the signature of some util funcs in the framework

e9953d1

caesarxuchao force-pushed the fix-defaut-gen branch from d6ea4d5 to e9953d1 Compare May 31, 2017 20:28

lavalamp merged commit c79c13d into kubernetes:master May 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default-gen changes for moving API types to k8s.io/api #54

Default-gen changes for moving API types to k8s.io/api #54

caesarxuchao commented May 12, 2017 •

edited

Loading

k8s-reviewable commented May 12, 2017

caesarxuchao commented May 12, 2017

caesarxuchao commented May 12, 2017

mbohlool commented May 15, 2017

caesarxuchao commented May 15, 2017

caesarxuchao commented May 30, 2017 •

edited

Loading

lavalamp May 30, 2017

caesarxuchao May 31, 2017

lavalamp May 31, 2017

caesarxuchao May 31, 2017

lavalamp May 31, 2017

caesarxuchao May 31, 2017

caesarxuchao May 31, 2017

lavalamp May 31, 2017

caesarxuchao May 31, 2017

caesarxuchao commented May 31, 2017

lavalamp May 31, 2017

lavalamp May 31, 2017

lavalamp commented May 31, 2017

Default-gen changes for moving API types to k8s.io/api #54

Default-gen changes for moving API types to k8s.io/api #54

Conversation

caesarxuchao commented May 12, 2017 • edited Loading

k8s-reviewable commented May 12, 2017

caesarxuchao commented May 12, 2017

caesarxuchao commented May 12, 2017

mbohlool commented May 15, 2017

caesarxuchao commented May 15, 2017

caesarxuchao commented May 30, 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

caesarxuchao commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented May 31, 2017

caesarxuchao commented May 12, 2017 •

edited

Loading

caesarxuchao commented May 30, 2017 •

edited

Loading