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

[Client-gen] Generate the clientset #19267

Merged
merged 1 commit into from
Jan 12, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion cmd/libs/go2idl/args/args.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func Default() *GeneratorArgs {
return generatorArgs
}

// GeneratorArgs has arguments common to most generators.
// GeneratorArgs has arguments that are passed to generators.
type GeneratorArgs struct {
// Which directories to parse.
InputDirs []string
Expand All @@ -61,6 +61,9 @@ type GeneratorArgs struct {

// If true, only verify, don't write anything.
VerifyOnly bool

Copy link
Contributor

Choose a reason for hiding this comment

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

This invalidates the comment above for GeneratorArgs. How about changing it to "GeneratorArgs has arguments that need to be passed to generators"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Will do.

// Any custom arguments go here
CustomArgs interface{}
}

func (g *GeneratorArgs) AddFlags(fs *pflag.FlagSet) {
Expand Down
61 changes: 58 additions & 3 deletions cmd/libs/go2idl/client-gen/generators/client-generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,29 @@ import (
"k8s.io/kubernetes/cmd/libs/go2idl/generator"
"k8s.io/kubernetes/cmd/libs/go2idl/namer"
"k8s.io/kubernetes/cmd/libs/go2idl/types"
"k8s.io/kubernetes/pkg/api/unversioned"

"github.com/golang/glog"
)

// ClientGenArgs is a wrapper for arguments to client-gen.
type ClientGenArgs struct {
// TODO: we should make another type declaration of GroupVersion out of the
// unversioned package, which is part of our API. Tools like client-gen
// shouldn't depend on an API.
GroupVersions []unversioned.GroupVersion
// ClientsetName is the name of the clientset to be generated. It's
// populated from command-line arguments.
ClientsetName string
// ClientsetOutputPath is the path the clientset will be generated at. It's
// populated from command-line arguments.
ClientsetOutputPath string
// ClientsetOnly determines if we should generate the clients for groups and
Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, for my own sake, pkg/client/typed/generated/ will not be updated if this is false. It will be updated if this is set to true (default), right?

Copy link
Member Author

Choose a reason for hiding this comment

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

No,

if ClientsetOnly=true, only pkg/client/clientset_generated will be updated,

if ClientsetOnly=false, both pkg/client/clientset_generated and pkg/client/typed/generated/ will be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes its reverse obviously :)
It might be good to mention that in the comment to make it clearer for everyone.
Your call.

// types along with the clientset. It's populated from command-line
// arguments.
ClientsetOnly bool
}

// NameSystems returns the name system used by the generators in this package.
func NameSystems() namer.NameSystems {
pluralExceptions := map[string]string{
Expand Down Expand Up @@ -110,6 +129,33 @@ func packageForGroup(group string, version string, typeList []*types.Type, packa
}
}

func packageForClientset(customArgs ClientGenArgs, typedClientBasePath string, boilerplate []byte) generator.Package {
return &generator.DefaultPackage{
PackageName: customArgs.ClientsetName,
PackagePath: filepath.Join(customArgs.ClientsetOutputPath, customArgs.ClientsetName),
HeaderText: boilerplate,
PackageDocumentation: []byte(
`// This package has the automatically generated clientset.
`),
// GeneratorFunc returns a list of generators. Each generator generates a
// single file.
GeneratorFunc: func(c *generator.Context) (generators []generator.Generator) {
generators = []generator.Generator{
&genClientset{
DefaultGen: generator.DefaultGen{
OptionalName: "clientset",
},
groupVersions: customArgs.GroupVersions,
typedClientPath: typedClientBasePath,
outputPackage: customArgs.ClientsetName,
imports: generator.NewImportTracker(),
},
}
return generators
},
}
}

// Packages makes the client package definition.
func Packages(context *generator.Context, arguments *args.GeneratorArgs) generator.Packages {
boilerplate, err := arguments.LoadGoBoilerplate()
Expand All @@ -136,11 +182,20 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
}
}

customArgs, ok := arguments.CustomArgs.(ClientGenArgs)
if !ok {
glog.Fatalf("cannot convert arguments.CustomArgs to ClientGenArgs")
}

var packageList []generator.Package
orderer := namer.Orderer{namer.NewPrivateNamer(0)}
for group, types := range groupToTypes {
packageList = append(packageList, packageForGroup(group, "unversioned", orderer.OrderTypes(types), arguments.OutputPackagePath, arguments.OutputBase, boilerplate))
// If --clientset-only=true, we don't regenerate the individual typed clients.
if !customArgs.ClientsetOnly {
orderer := namer.Orderer{namer.NewPrivateNamer(0)}
for group, types := range groupToTypes {
packageList = append(packageList, packageForGroup(group, "unversioned", orderer.OrderTypes(types), arguments.OutputPackagePath, arguments.OutputBase, boilerplate))
}
}

packageList = append(packageList, packageForClientset(customArgs, arguments.OutputPackagePath, boilerplate))
return generator.Packages(packageList)
}
174 changes: 174 additions & 0 deletions cmd/libs/go2idl/client-gen/generators/generator-for-clientset.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
/*
Copyright 2016 The Kubernetes Authors All rights reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package generators

import (
Copy link
Contributor

Choose a reason for hiding this comment

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

Any tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you are referring to the tests for the generated clientset, not the for generator itself, right? Can I add them when in another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was referring to tests for this generator code.

Copy link
Member Author

Choose a reason for hiding this comment

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

cmd/libs/go2idl/client-gen/testoutput/clientset_generated/test_release_1_1/clientset.go is a test for the generator code, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is like the integration test :)
I was referring to unit test for functions in this file.

To be honest, another motivation for asking for tests was to see how the functions in this file should be used.
Like I should call Filter() first and then GenerateType() and not the other way round.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be honest, another motivation for asking for tests was to see how the functions in this file should be used.
Like I should call Filter() first and then GenerateType() and not the other way round.

That is a framework thing, IMO it should be the job for the framework rather than for each individual generator.

That is like the integration test

Yes, kind of, but writing unit test for each function would be too trivial.

"fmt"
"io"
"path/filepath"

"k8s.io/kubernetes/cmd/libs/go2idl/generator"
"k8s.io/kubernetes/cmd/libs/go2idl/namer"
"k8s.io/kubernetes/cmd/libs/go2idl/types"
"k8s.io/kubernetes/pkg/api/unversioned"
)

// genClientset generates a package for a clientset.
type genClientset struct {
generator.DefaultGen
groupVersions []unversioned.GroupVersion
typedClientPath string
outputPackage string
imports *generator.ImportTracker
}

var _ generator.Generator = &genClientset{}

func (g *genClientset) Namers(c *generator.Context) namer.NameSystems {
return namer.NameSystems{
"raw": namer.NewRawNamer(g.outputPackage, g.imports),
}
}

var generate_clientset = true

// We only want to call GenerateType() once.
func (g *genClientset) Filter(c *generator.Context, t *types.Type) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

No idea what this function is doing :)
Can you please explain a bit more?

Copy link
Member Author

Choose a reason for hiding this comment

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

The go2idl framework will parse all the types in your input directory, and the imported packages, and then the framework iterate through all the types, use the Filter() to filter the types, and then call the GenerateType(type).

Here, we actually don't care about type, we just need to make sure GenerateType() is called once, and note that GenerateType doesn't use any type-specific information.

In short, the framework is built around types, while clientset is not type-specific, so there is some workaround in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whats wrong if GenerateType is called twice?
What if I want to regenerate it?

If we do want to restrict it, the comment for GenerateType should say that it can only be called once and maybe the function should give an error if called multiple times?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whats wrong if GenerateType is called twice?

Nothing wrong, it's just that the same file will be regenerated with the same content many times. Pure inefficiency.

What if I want to regenerate it?

You can run client-gen in your terminal again. There is no reason to generate the clientset multiple times in one run of client-gen.

If we do want to restrict it, the comment for GenerateType should say that it can only be called once and maybe the function should give an error if called multiple times?

It's not an error to call GenerateType multiple times, it's just inefficient. I prefer to make the restrictriction in Filter(), we have a similar practice here.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can run client-gen in your terminal again. There is no reason to generate the clientset multiple times in one run of client-gen.

How will GenerateType() be called multiple times in the same run?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment at the top of this file (below line 29) that how the functions in this file should be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the go2idl framework iterate through all the types it has parsed, and call GenerateType() for every type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

aah those comments are awesome!
As discussed, make it explicit that genClientset implements that interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that would make the code much easier to understand. Thanks for the suggestion.

ret := generate_clientset
generate_clientset = false
return ret
}

func normalizeGroup(group string) string {
if group == "api" {
return "legacy"
}
return group
}

func normalizeVersion(version string) string {
if version == "" {
return "unversioned"
}
return version
}

func (g *genClientset) Imports(c *generator.Context) (imports []string) {
for _, gv := range g.groupVersions {
group := normalizeGroup(gv.Group)
version := normalizeVersion(gv.Version)
typedClientPath := filepath.Join(g.typedClientPath, group, version)
imports = append(imports, g.imports.ImportLines()...)
imports = append(imports, fmt.Sprintf("%s_%s \"%s\"", group, version, typedClientPath))
}
return
}

func (g *genClientset) GenerateType(c *generator.Context, t *types.Type, w io.Writer) error {
// TODO: We actually don't need any type information to generate the clientset,
// perhaps we can adapt the go2ild framework to this kind of usage.
sw := generator.NewSnippetWriter(w, c, "$", "$")
const pkgUnversioned = "k8s.io/kubernetes/pkg/client/unversioned"

type arg struct {
Group string
PackageName string
}

allGroups := []arg{}
for _, gv := range g.groupVersions {
group := normalizeGroup(gv.Group)
version := normalizeVersion(gv.Version)
allGroups = append(allGroups, arg{namer.IC(group), group + "_" + version})
}

m := map[string]interface{}{
"allGroups": allGroups,
"Config": c.Universe.Type(types.Name{Package: pkgUnversioned, Name: "Config"}),
"DefaultKubernetesUserAgent": c.Universe.Function(types.Name{Package: pkgUnversioned, Name: "DefaultKubernetesUserAgent"}),
"RESTClient": c.Universe.Type(types.Name{Package: pkgUnversioned, Name: "RESTClient"}),
}
sw.Do(clientsetInterfaceTemplate, m)
sw.Do(clientsetTemplate, m)
for _, g := range allGroups {
sw.Do(clientsetInterfaceImplTemplate, g)
}
sw.Do(newClientsetForConfigTemplate, m)
sw.Do(newClientsetForConfigOrDieTemplate, m)
sw.Do(newClientsetForRESTClientTemplate, m)

return sw.Error()
}

var clientsetInterfaceTemplate = `
type Interface interface {
$range .allGroups$$.Group$() $.PackageName$.$.Group$Client
$end$
}
`

var clientsetTemplate = `
// Clientset contains the clients for groups. Each group has exactly one
// version included in a Clientset.
type Clientset struct {
$range .allGroups$*$.PackageName$.$.Group$Client
$end$
}
`

var clientsetInterfaceImplTemplate = `
// $.Group$ retrieves the $.Group$Client
func (c *Clientset) $.Group$() *$.PackageName$.$.Group$Client {
return c.$.Group$Client
}
`

var newClientsetForConfigTemplate = `
// NewForConfig creates a new Clientset for the given config.
func NewForConfig(c *$.Config|raw$) (*Clientset, error) {
var clientset Clientset
var err error
$range .allGroups$ clientset.$.Group$Client, err =$.PackageName$.NewForConfig(c)
if err!=nil {
return nil, err
}
$end$
return &clientset, nil
}
`

var newClientsetForConfigOrDieTemplate = `
// NewForConfigOrDie creates a new Clientset for the given config and
// panics if there is an error in the config.
func NewForConfigOrDie(c *$.Config|raw$) *Clientset {
var clientset Clientset
$range .allGroups$ clientset.$.Group$Client =$.PackageName$.NewForConfigOrDie(c)
$end$
return &clientset
}
`

var newClientsetForRESTClientTemplate = `
// New creates a new Clientset for the given RESTClient.
func New(c *$.RESTClient|raw$) *Clientset {
var clientset Clientset
$range .allGroups$ clientset.$.Group$Client =$.PackageName$.New(c)
$end$

return &clientset
}
`
2 changes: 2 additions & 0 deletions cmd/libs/go2idl/client-gen/generators/generator-for-group.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ type genGroup struct {
imports *generator.ImportTracker
}

var _ generator.Generator = &genGroup{}

// We only want to call GenerateType() once per group.
func (g *genGroup) Filter(c *generator.Context, t *types.Type) bool {
return t == g.types[0]
Expand Down
2 changes: 2 additions & 0 deletions cmd/libs/go2idl/client-gen/generators/generator-for-type.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ type genClientForType struct {
imports *generator.ImportTracker
}

var _ generator.Generator = &genClientForType{}

// Filter ignores all but one type because we're making a single file per type.
func (g *genClientForType) Filter(c *generator.Context, t *types.Type) bool { return t == g.typeToMatch }

Expand Down