From e6bc813ac3710da446cf5d540bcab60ab078c7ed Mon Sep 17 00:00:00 2001 From: Joe Betz Date: Fri, 8 May 2026 18:50:16 -0400 Subject: [PATCH] Improve error messages and documentation --- .../generators/targets.go | 7 +- .../client-gen/generators/client_generator.go | 7 +- .../generators/generator_for_group.go | 7 +- .../cmd/informer-gen/generators/targets.go | 7 +- .../cmd/lister-gen/generators/lister.go | 7 +- .../cmd/register-gen/generators/targets.go | 7 +- .../pkg/apidefinitions/activation_test.go | 7 +- .../code-generator/pkg/apidefinitions/lint.go | 72 ++++++++++++++----- .../code-generator/pkg/apidefinitions/tags.go | 38 ++++++---- .../pkg/apidefinitions/tags_test.go | 20 +++--- 10 files changed, 108 insertions(+), 71 deletions(-) diff --git a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/targets.go b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/targets.go index 01a8a3f1966..73c3f2cb27c 100644 --- a/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/targets.go +++ b/staging/src/k8s.io/code-generator/cmd/applyconfiguration-gen/generators/targets.go @@ -17,7 +17,6 @@ limitations under the License. package generators import ( - "errors" "fmt" "path" "path/filepath" @@ -294,11 +293,11 @@ func groupVersion(p *types.Package) (gv clientgentypes.GroupVersion, err error) // If there's a comment of the form "// +groupName=somegroup" or // "// +groupName=somegroup.foo.bar.io", use the first field (somegroup) as the name of the // group when generating. - override, err := apidefinitions.GroupNameForPackage(p.Comments) - if err != nil && !errors.Is(err, apidefinitions.ErrGroupUndeclared) { + override, ok, err := apidefinitions.GroupNameForPackage(p.Comments) + if err != nil { return gv, err } - if err == nil { + if ok { gv.Group = clientgentypes.Group(override) } diff --git a/staging/src/k8s.io/code-generator/cmd/client-gen/generators/client_generator.go b/staging/src/k8s.io/code-generator/cmd/client-gen/generators/client_generator.go index 9bbcdee6bfe..65dafc82ecf 100644 --- a/staging/src/k8s.io/code-generator/cmd/client-gen/generators/client_generator.go +++ b/staging/src/k8s.io/code-generator/cmd/client-gen/generators/client_generator.go @@ -18,7 +18,6 @@ limitations under the License. package generators import ( - "errors" "fmt" "path" "path/filepath" @@ -281,11 +280,11 @@ func applyGroupOverrides(universe types.Universe, args *args.Args) error { changes := make(map[clientgentypes.GroupVersion]clientgentypes.GroupVersion) for gv, inputDir := range args.GroupVersionPackages() { p := universe.Package(inputDir) - override, err := apidefinitions.GroupNameForPackage(p.Comments) - if err != nil && !errors.Is(err, apidefinitions.ErrGroupUndeclared) { + override, ok, err := apidefinitions.GroupNameForPackage(p.Comments) + if err != nil { return err } - if err == nil { + if ok { newGV := clientgentypes.GroupVersion{ Group: clientgentypes.Group(override), Version: gv.Version, diff --git a/staging/src/k8s.io/code-generator/cmd/client-gen/generators/generator_for_group.go b/staging/src/k8s.io/code-generator/cmd/client-gen/generators/generator_for_group.go index c20e4ea9633..ae86da83805 100644 --- a/staging/src/k8s.io/code-generator/cmd/client-gen/generators/generator_for_group.go +++ b/staging/src/k8s.io/code-generator/cmd/client-gen/generators/generator_for_group.go @@ -17,7 +17,6 @@ limitations under the License. package generators import ( - "errors" "io" "path" @@ -73,11 +72,11 @@ func (g *genGroup) GenerateType(c *generator.Context, t *types.Type, w io.Writer // allow user to define a group name that's different from the one parsed from the directory. p := c.Universe.Package(g.inputPackage) groupName := g.group - override, err := apidefinitions.GroupNameForPackage(p.Comments) - if err != nil && !errors.Is(err, apidefinitions.ErrGroupUndeclared) { + override, ok, err := apidefinitions.GroupNameForPackage(p.Comments) + if err != nil { return err } - if err == nil { + if ok { groupName = override } diff --git a/staging/src/k8s.io/code-generator/cmd/informer-gen/generators/targets.go b/staging/src/k8s.io/code-generator/cmd/informer-gen/generators/targets.go index 62790012806..0f916d6bf1e 100644 --- a/staging/src/k8s.io/code-generator/cmd/informer-gen/generators/targets.go +++ b/staging/src/k8s.io/code-generator/cmd/informer-gen/generators/targets.go @@ -17,7 +17,6 @@ limitations under the License. package generators import ( - "errors" "fmt" "path" "path/filepath" @@ -159,11 +158,11 @@ func GetTargets(context *generator.Context, args *args.Args) []generator.Target // If there's a comment of the form "// +groupName=somegroup" or // "// +groupName=somegroup.foo.bar.io", use the first field (somegroup) as the name of the // group when generating. - override, err := apidefinitions.GroupNameForPackage(p.Comments) - if err != nil && !errors.Is(err, apidefinitions.ErrGroupUndeclared) { + override, ok, err := apidefinitions.GroupNameForPackage(p.Comments) + if err != nil { klog.Fatalf("error resolving group name: %v", err) } - if err == nil { + if ok { gv.Group = clientgentypes.Group(override) } diff --git a/staging/src/k8s.io/code-generator/cmd/lister-gen/generators/lister.go b/staging/src/k8s.io/code-generator/cmd/lister-gen/generators/lister.go index fb8785d2c99..8f9623bcb2e 100644 --- a/staging/src/k8s.io/code-generator/cmd/lister-gen/generators/lister.go +++ b/staging/src/k8s.io/code-generator/cmd/lister-gen/generators/lister.go @@ -17,7 +17,6 @@ limitations under the License. package generators import ( - "errors" "fmt" "io" "path" @@ -116,11 +115,11 @@ func GetTargets(context *generator.Context, args *args.Args) []generator.Target // If there's a comment of the form "// +groupName=somegroup" or // "// +groupName=somegroup.foo.bar.io", use the first field (somegroup) as the name of the // group when generating. - override, err := apidefinitions.GroupNameForPackage(p.Comments) - if err != nil && !errors.Is(err, apidefinitions.ErrGroupUndeclared) { + override, ok, err := apidefinitions.GroupNameForPackage(p.Comments) + if err != nil { klog.Fatalf("error resolving group name: %v", err) } - if err == nil { + if ok { gv.Group = clientgentypes.Group(strings.SplitN(override, ".", 2)[0]) } diff --git a/staging/src/k8s.io/code-generator/cmd/register-gen/generators/targets.go b/staging/src/k8s.io/code-generator/cmd/register-gen/generators/targets.go index ea8bdc4d8a9..9d66e20d96f 100644 --- a/staging/src/k8s.io/code-generator/cmd/register-gen/generators/targets.go +++ b/staging/src/k8s.io/code-generator/cmd/register-gen/generators/targets.go @@ -17,7 +17,6 @@ limitations under the License. package generators import ( - "errors" "fmt" "os" "path" @@ -93,11 +92,11 @@ func GetTargets(context *generator.Context, args *args.Args) []generator.Target // if there is a comment of the form "// +groupName=somegroup" or "// +groupName=somegroup.foo.bar.io", // extract the fully qualified API group name from it and overwrite the group inferred from the package path - override, err := apidefinitions.GroupNameForPackage(pkg.Comments) - if err != nil && !errors.Is(err, apidefinitions.ErrGroupUndeclared) { + override, ok, err := apidefinitions.GroupNameForPackage(pkg.Comments) + if err != nil { klog.Fatalf("error resolving group name: %v", err) } - if err == nil { + if ok { klog.V(5).Infof("overriding the group name with = %s", override) gv.Group = clientgentypes.Group(override) } diff --git a/staging/src/k8s.io/code-generator/pkg/apidefinitions/activation_test.go b/staging/src/k8s.io/code-generator/pkg/apidefinitions/activation_test.go index c0decf208f5..f74586a0a26 100644 --- a/staging/src/k8s.io/code-generator/pkg/apidefinitions/activation_test.go +++ b/staging/src/k8s.io/code-generator/pkg/apidefinitions/activation_test.go @@ -108,14 +108,13 @@ func TestIdentify(t *testing.T) { comments: []string{"+k8s:prerelease-lifecycle-gen=false"}, }, { - name: "=false mixed with peer pkg passes through", + name: "=false mixed with peer pkg is rejected", spec: Conversion, comments: []string{ "+k8s:conversion-gen=k8s.io/api/extensions/v1beta1", "+k8s:conversion-gen=false", }, - wantShouldGenerate: true, - wantValues: []string{"k8s.io/api/extensions/v1beta1", "false"}, + wantErr: `cannot mix "false"`, }, { name: "repeated identical peer is allowed and preserved", @@ -148,7 +147,7 @@ func TestIdentify(t *testing.T) { comments: []string{ "+k8s:conversion-x-gen=k8s.io/api/foo", }, - wantErr: "+k8s:conversion--gen", + wantErr: "+k8s:conversion-x-gen", }, { name: "non-generator +k8s: tag passes validation untouched", diff --git a/staging/src/k8s.io/code-generator/pkg/apidefinitions/lint.go b/staging/src/k8s.io/code-generator/pkg/apidefinitions/lint.go index eb5be29daa4..a76449e12b9 100644 --- a/staging/src/k8s.io/code-generator/pkg/apidefinitions/lint.go +++ b/staging/src/k8s.io/code-generator/pkg/apidefinitions/lint.go @@ -17,6 +17,7 @@ limitations under the License. package apidefinitions import ( + "errors" "fmt" "path" "reflect" @@ -146,18 +147,41 @@ func checkExplicitDisablement(pkg *types.Package) error { return nil } tagged := codetags.Extract("+", pkg.Comments) - var missing []string + var missing []Spec for _, spec := range expected { if _, ok := tagged[spec.ActivationTag]; !ok { - missing = append(missing, "+"+spec.ActivationTag) + missing = append(missing, spec) } } if len(missing) == 0 { return nil } - sort.Strings(missing) - return fmt.Errorf("%s/doc.go: package classifies as %s but lacks explicit +k8s:=value or =false for: %s", - pkg.Dir, roles, strings.Join(missing, ", ")) + sort.Slice(missing, func(i, j int) bool { return missing[i].ActivationTag < missing[j].ActivationTag }) + + var sb strings.Builder + fmt.Fprintf(&sb, "%s/doc.go: %s.\n", pkg.Dir, roles.describePackage(pkg.Dir)) + sb.WriteString("Set explicit configuration or disablement for these generators in doc.go:") + for _, spec := range missing { + fmt.Fprintf(&sb, "\n +%s=<%s>", spec.ActivationTag, valueHint(spec)) + } + return errors.New(sb.String()) +} + +// valueHint describes the accepted shape of an ActivationTag value, for +// inclusion in user-facing errors. +func valueHint(spec Spec) string { + switch spec.ValueMode { + case ConversionPeerList: + return "peer-package or false" + case TypeFilterList: + return "TypeMeta or false" + case Package: + return "package or false" + case Boolean: + return "true or false" + default: + return "value or false" + } } func expectedGenerators(roles packageRoles) []Spec { @@ -172,31 +196,47 @@ func expectedGenerators(roles packageRoles) []Spec { } type packageRoles struct { - // Not all packages have a single role. Combinations are allowed. - isExternalGroup bool - isInternalGroup bool + // isExternalGroup identifies an API group package containing + // versioned API types (e.g. staging/src/k8s.io/api/apps). + isExternalGroup bool + + // isInternalGroup identifies an API group package containing + // API type internals (e.g. pkg/apis/apps). The "internal type" (aka Hub type) + // often lives here. + isInternalGroup bool + + // isExternalVersion identifies a package containing versioned API types + // (e.g. staging/src/k8s.io/api/apps/v1). isExternalVersion bool + + // isInternalVersion identifies a per-version internals package containing + // conversion / defaulting / validation for hub types in the parent + // directory (e.g. pkg/apis/apps/v1). isInternalVersion bool } -func (r packageRoles) String() string { +func (r packageRoles) describePackage(pkgDir string) string { var parts []string if r.isExternalGroup { - parts = append(parts, "external-group") + parts = append(parts, "API group package with versioned API types in subpackages") } if r.isInternalGroup { - parts = append(parts, "internal-group") + parts = append(parts, "API group package containing hub API types") } if r.isExternalVersion { - parts = append(parts, "external-version") + parts = append(parts, "package containing versioned API types") } if r.isInternalVersion { - parts = append(parts, "internal-version") + desc := "package containing per-version internals (conversion, defaulting, validation)" + if parent := path.Dir(pkgDir); parent != "" && parent != "." && parent != "/" { + desc += " for hub types at " + parent + } + parts = append(parts, desc) } if len(parts) == 0 { - return "not-api-package" + return "package has no recognized API role" } - return strings.Join(parts, ",") + return "this package looks like: " + strings.Join(parts, "; ") } // classifyPackage looks for evidence of package roles. @@ -236,7 +276,7 @@ func classifyPackage(pkg *types.Package) packageRoles { } func looksLikeAPIPackage(pkg *types.Package) bool { - if _, err := GroupNameForPackage(pkg.Comments); err == nil { + if _, ok, _ := GroupNameForPackage(pkg.Comments); ok { return true } for name := range codetags.Extract("+", pkg.Comments) { diff --git a/staging/src/k8s.io/code-generator/pkg/apidefinitions/tags.go b/staging/src/k8s.io/code-generator/pkg/apidefinitions/tags.go index 94acab9608a..c879a1bff2f 100644 --- a/staging/src/k8s.io/code-generator/pkg/apidefinitions/tags.go +++ b/staging/src/k8s.io/code-generator/pkg/apidefinitions/tags.go @@ -17,7 +17,7 @@ limitations under the License. package apidefinitions import ( - "errors" + "fmt" "k8s.io/gengo/v2/codetags" ) @@ -25,8 +25,9 @@ import ( const tagGroupName = "groupName" // extractGeneratorTag returns the values of "+" in comments. -// A sole "false" value sets optedOut and clears values; any other value -// (including "false" mixed with other values) is returned as-is. +// If any value is "false", every value must be "false" (duplicate +// disablement is allowed but mixing disablement with any other value +// is an error); optedOut is set when so. func extractGeneratorTag(comments []string, tagName string) (values []string, optedOut bool, err error) { if tagName == "" { return nil, false, nil @@ -35,7 +36,18 @@ func extractGeneratorTag(comments []string, tagName string) (values []string, op if err != nil { return nil, false, err } - if len(values) == 1 && values[0] == "false" { + hasFalse, hasOther := false, false + for _, v := range values { + if v == "false" { + hasFalse = true + } else { + hasOther = true + } + } + if hasFalse && hasOther { + return nil, false, fmt.Errorf("+%s: cannot mix \"false\" with other values; got %v", tagName, values) + } + if hasFalse { return nil, true, nil } return values, false, nil @@ -71,19 +83,15 @@ func extractTags(lines []string, names ...string) (map[string][]codetags.Tag, er return out, nil } -// ErrGroupUndeclared indicates that no +groupName= tag declares a group -// for the package. -var ErrGroupUndeclared = errors.New("no group declared for package") - -// GroupNameForPackage returns the +groupName= value from comments, or -// ErrGroupUndeclared if no such tag is present. -func GroupNameForPackage(comments []string) (string, error) { +// GroupNameForPackage returns the value of the +groupName= tag from +// comments. ok is false when the tag is absent. +func GroupNameForPackage(comments []string) (group string, ok bool, err error) { values, err := tagValues(comments, tagGroupName) if err != nil { - return "", err + return "", false, err } - if len(values) > 0 { - return values[0], nil + if len(values) == 0 { + return "", false, nil } - return "", ErrGroupUndeclared + return values[0], true, nil } diff --git a/staging/src/k8s.io/code-generator/pkg/apidefinitions/tags_test.go b/staging/src/k8s.io/code-generator/pkg/apidefinitions/tags_test.go index 8e6533a4c9c..495aad01e04 100644 --- a/staging/src/k8s.io/code-generator/pkg/apidefinitions/tags_test.go +++ b/staging/src/k8s.io/code-generator/pkg/apidefinitions/tags_test.go @@ -17,7 +17,6 @@ limitations under the License. package apidefinitions import ( - "errors" "testing" ) @@ -26,24 +25,21 @@ func TestGroupNameForPackage(t *testing.T) { name string comments []string want string - wantErr error + wantOK bool }{ - {name: "groupName tag", comments: []string{"+groupName=apps"}, want: "apps"}, - {name: "qualified groupName", comments: []string{"+groupName=foo.example.com"}, want: "foo.example.com"}, - {name: "no tag", wantErr: ErrGroupUndeclared}, + {name: "groupName tag", comments: []string{"+groupName=apps"}, want: "apps", wantOK: true}, + {name: "qualified groupName", comments: []string{"+groupName=foo.example.com"}, want: "foo.example.com", wantOK: true}, + {name: "no tag"}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got, err := GroupNameForPackage(tc.comments) - if tc.wantErr != nil { - if !errors.Is(err, tc.wantErr) { - t.Errorf("err = %v, want %v", err, tc.wantErr) - } - return - } + got, ok, err := GroupNameForPackage(tc.comments) if err != nil { t.Fatalf("err = %v", err) } + if ok != tc.wantOK { + t.Errorf("ok = %v, want %v", ok, tc.wantOK) + } if got != tc.want { t.Errorf("got %q, want %q", got, tc.want) }