From 10ba6345227e3444475029bad35819c6b0fd085b Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Wed, 10 Feb 2021 16:52:18 -0800 Subject: [PATCH] internal/generate/clusterserviceversion/bases/definitions: consider both untyped and Typ-typed *ast.Object's that underly an *ast.Ident, such that types in other files are found properly Signed-off-by: Eric Stroczynski --- changelog/fragments/bugfix-3744.yaml | 4 + .../bases/definitions/ast.go | 81 ++++++++----------- .../bases/definitions/crd.go | 2 +- .../bases/definitions/crd_test.go | 25 +++--- .../testdata/go/api/v1alpha2/dummy_types.go | 66 --------------- .../go/api/v1alpha2/motorcycle_types.go | 81 +++++++++++++++++++ 6 files changed, 132 insertions(+), 127 deletions(-) create mode 100644 changelog/fragments/bugfix-3744.yaml create mode 100644 internal/generate/testdata/go/api/v1alpha2/motorcycle_types.go diff --git a/changelog/fragments/bugfix-3744.yaml b/changelog/fragments/bugfix-3744.yaml new file mode 100644 index 0000000000..e93ed1128a --- /dev/null +++ b/changelog/fragments/bugfix-3744.yaml @@ -0,0 +1,4 @@ +entries: + - description: > + Properly consider all Go files when generating a CSV's `spec.customresourcedefinitions.owned`. + kind: bugfix diff --git a/internal/generate/clusterserviceversion/bases/definitions/ast.go b/internal/generate/clusterserviceversion/bases/definitions/ast.go index b3717b6799..c411e7d09b 100644 --- a/internal/generate/clusterserviceversion/bases/definitions/ast.go +++ b/internal/generate/clusterserviceversion/bases/definitions/ast.go @@ -19,8 +19,8 @@ import ( "fmt" "go/ast" "strconv" - "strings" + "golang.org/x/tools/go/packages" "sigs.k8s.io/controller-tools/pkg/crd" "sigs.k8s.io/controller-tools/pkg/loader" "sigs.k8s.io/controller-tools/pkg/markers" @@ -79,20 +79,21 @@ func (im importIdents) findPackagePathForSelExpr(expr *ast.SelectorExpr) (pkgPat // Short-circuit if only one import. if len(imports) == 1 { - return imports[0].path - } - - // If multiple files contain the same local import name, check to see which file contains the selector expression. - for _, imp := range imports { - if imp.f.Pos() <= expr.Pos() && imp.f.End() >= expr.End() { - return imp.path + pkgPath = imports[0].path + } else { + // If multiple files contain the same local import name, check to see which file contains the selector expression. + for _, imp := range imports { + if imp.f.Pos() <= expr.Pos() && imp.f.End() >= expr.End() { + pkgPath = imp.path + break + } } } - return "" + return loader.NonVendorPath(pkgPath) } // getMarkedChildrenOfField collects all marked fields from type declarations starting at rootField in depth-first order. -func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField markers.FieldInfo) (map[string][]*fieldInfo, error) { +func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField markers.FieldInfo) (map[crd.TypeIdent][]*fieldInfo, error) { // Gather all types and imports needed to build the BFS tree. rootPkg.NeedTypesInfo() importIDs, err := newImportIdents(rootPkg) @@ -102,41 +103,37 @@ func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField m // ast.Inspect will not traverse into fields, so iteratively collect them and to check for markers. nextFields := []*fieldInfo{{FieldInfo: rootField}} - markedFields := map[string][]*fieldInfo{} + markedFields := map[crd.TypeIdent][]*fieldInfo{} for len(nextFields) > 0 { fields := []*fieldInfo{} for _, field := range nextFields { - errs := []error{} ast.Inspect(field.RawField, func(n ast.Node) bool { if n == nil { return true } - var info *markers.TypeInfo - var hasInfo bool + var ident crd.TypeIdent switch nt := n.(type) { - case *ast.SelectorExpr: - // Case of a type definition in an imported package. - + case *ast.SelectorExpr: // Type definition in an imported package. pkgPath := importIDs.findPackagePathForSelExpr(nt) if pkgPath == "" { // Found no reference to pkgPath in any file. return true } - if pkg, hasImport := rootPkg.Imports()[loader.NonVendorPath(pkgPath)]; hasImport { - // Check if the field's type exists in the known types. - info, hasInfo = g.types[crd.TypeIdent{Package: pkg, Name: nt.Sel.Name}] + if pkg, hasImport := rootPkg.Imports()[pkgPath]; hasImport { + pkg.NeedTypesInfo() + ident = crd.TypeIdent{Package: pkg, Name: nt.Sel.Name} } - case *ast.Ident: - // Case of a local type definition. - - // Only look at type names. - if nt.Obj != nil && nt.Obj.Kind == ast.Typ { - // Check if the field's type exists in the known types. - info, hasInfo = g.types[crd.TypeIdent{Package: rootPkg, Name: nt.Name}] + case *ast.Ident: // Local type definition. + // Only look at type idents or references to type idents in other files. + if nt.Obj == nil || nt.Obj.Kind == ast.Typ { + ident = crd.TypeIdent{Package: rootPkg, Name: nt.Name} } } - if !hasInfo { + + // Check if the field's type is a known types. + info, hasInfo := g.types[ident] + if ident == (crd.TypeIdent{}) || !hasInfo { return true } @@ -144,8 +141,8 @@ func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField m for _, finfo := range info.Fields { segment, err := getPathSegmentForField(finfo) if err != nil { - errs = append(errs, fmt.Errorf("error getting path from type %s field %s: %v", info.Name, finfo.Name, err)) - return true + rootPkg.AddError(fmt.Errorf("error getting path from type %s field %s: %v", info.Name, finfo.Name, err)) + continue } // Add extra information to the segment if it comes from a certain field type. switch finfo.RawField.Type.(type) { @@ -166,33 +163,19 @@ func (g generator) getMarkedChildrenOfField(rootPkg *loader.Package, rootField m fields = append(fields, f) // Marked fields get collected for the caller to parse. if len(finfo.Markers) != 0 { - markedFields[info.Name] = append(markedFields[info.Name], f) + markedFields[ident] = append(markedFields[ident], f) } } return true }) - if err := fmtParseErrors(errs); err != nil { - return nil, err - } } nextFields = fields } - return markedFields, nil -} -// fmtParseErrors prettifies a list of errors to make them easier to read. -func fmtParseErrors(errs []error) error { - switch len(errs) { - case 0: - return nil - case 1: - return errs[0] + if loader.PrintErrors([]*loader.Package{rootPkg}, packages.TypeError) { + return nil, errors.New("package had type errors") } - sb := strings.Builder{} - for _, err := range errs { - sb.WriteString("\n") - sb.WriteString(err.Error()) - } - return errors.New(sb.String()) + + return markedFields, nil } diff --git a/internal/generate/clusterserviceversion/bases/definitions/crd.go b/internal/generate/clusterserviceversion/bases/definitions/crd.go index 0f0c01036f..b84e4adae6 100644 --- a/internal/generate/clusterserviceversion/bases/definitions/crd.go +++ b/internal/generate/clusterserviceversion/bases/definitions/crd.go @@ -133,7 +133,7 @@ func (g generator) getTypedDescriptors(pkg *loader.Package, kindType *markers.Ty return getTypedDescriptors(markedFields, t, descType), nil } -func getTypedDescriptors(markedFields map[string][]*fieldInfo, t reflect.Type, descType string) (descriptors []interface{}) { +func getTypedDescriptors(markedFields map[crd.TypeIdent][]*fieldInfo, t reflect.Type, descType string) (descriptors []interface{}) { descriptorBuckets := make(map[int][]reflect.Value) orders := make([]int, 0) for _, fields := range markedFields { diff --git a/internal/generate/clusterserviceversion/bases/definitions/crd_test.go b/internal/generate/clusterserviceversion/bases/definitions/crd_test.go index 37426574fe..193b48eb05 100644 --- a/internal/generate/clusterserviceversion/bases/definitions/crd_test.go +++ b/internal/generate/clusterserviceversion/bases/definitions/crd_test.go @@ -24,18 +24,20 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "github.com/operator-framework/api/pkg/operators/v1alpha1" + "sigs.k8s.io/controller-tools/pkg/crd" + "sigs.k8s.io/controller-tools/pkg/loader" "sigs.k8s.io/controller-tools/pkg/markers" "sigs.k8s.io/kubebuilder/v2/test/e2e/utils" ) var _ = Describe("getTypedDescriptors", func() { var ( - markedFields map[string][]*fieldInfo + markedFields map[crd.TypeIdent][]*fieldInfo out []interface{} ) BeforeEach(func() { - markedFields = make(map[string][]*fieldInfo) + markedFields = make(map[crd.TypeIdent][]*fieldInfo) }) It("handles an empty set of marked fields", func() { @@ -43,7 +45,7 @@ var _ = Describe("getTypedDescriptors", func() { Expect(out).To(HaveLen(0)) }) It("returns one spec descriptor for one spec marker on a field", func() { - markedFields[""] = []*fieldInfo{ + markedFields[crd.TypeIdent{}] = []*fieldInfo{ { FieldInfo: markers.FieldInfo{ Markers: markers.MarkerValues{ @@ -64,7 +66,7 @@ var _ = Describe("getTypedDescriptors", func() { })) }) It("returns no spec descriptors for one status marker on a field", func() { - markedFields[""] = []*fieldInfo{ + markedFields[crd.TypeIdent{}] = []*fieldInfo{ { FieldInfo: markers.FieldInfo{ Markers: markers.MarkerValues{ @@ -79,7 +81,7 @@ var _ = Describe("getTypedDescriptors", func() { Expect(out).To(HaveLen(0)) }) It("returns one status descriptor for one status marker on a field", func() { - markedFields[""] = []*fieldInfo{ + markedFields[crd.TypeIdent{}] = []*fieldInfo{ { FieldInfo: markers.FieldInfo{ Markers: markers.MarkerValues{ @@ -100,7 +102,7 @@ var _ = Describe("getTypedDescriptors", func() { })) }) It("returns one spec descriptor for three spec markers and one status marker on a field", func() { - markedFields[""] = []*fieldInfo{ + markedFields[crd.TypeIdent{}] = []*fieldInfo{ { FieldInfo: markers.FieldInfo{ Markers: markers.MarkerValues{ @@ -126,7 +128,7 @@ var _ = Describe("getTypedDescriptors", func() { })) }) It("returns two spec descriptor for spec markers on two different fields", func() { - markedFields[""] = []*fieldInfo{ + markedFields[crd.TypeIdent{}] = []*fieldInfo{ { FieldInfo: markers.FieldInfo{ Markers: markers.MarkerValues{ @@ -163,10 +165,10 @@ func intPtr(i int) *int { return &i } // makeMockMarkedFields returns a randomly generated mock marked field set, // and the expected sorted set of descriptors. -func makeMockMarkedFields() (markedFields map[string][]*fieldInfo, expected []interface{}) { +func makeMockMarkedFields() (markedFields map[crd.TypeIdent][]*fieldInfo, expected []interface{}) { descBuckets := make(map[int][]v1alpha1.SpecDescriptor, 100) r := rand.New(rand.NewSource(time.Now().UnixNano())) - markedFields = make(map[string][]*fieldInfo, 100) + markedFields = make(map[crd.TypeIdent][]*fieldInfo, 100) for i := 0; i < 100; i++ { s, err := utils.RandomSuffix() if err != nil { @@ -174,10 +176,11 @@ func makeMockMarkedFields() (markedFields map[string][]*fieldInfo, expected []in } name := strings.Title(s) order := r.Int() % 200 // Very likely to get one conflict. - if _, hasName := markedFields[name]; hasName { + ident := crd.TypeIdent{Package: &loader.Package{}, Name: name} + if _, hasName := markedFields[ident]; hasName { continue } - markedFields[name] = []*fieldInfo{ + markedFields[ident] = []*fieldInfo{ { FieldInfo: markers.FieldInfo{ Markers: markers.MarkerValues{ diff --git a/internal/generate/testdata/go/api/v1alpha2/dummy_types.go b/internal/generate/testdata/go/api/v1alpha2/dummy_types.go index de32b5fe3f..60a9c5ac50 100644 --- a/internal/generate/testdata/go/api/v1alpha2/dummy_types.go +++ b/internal/generate/testdata/go/api/v1alpha2/dummy_types.go @@ -68,72 +68,6 @@ type DummyStatus struct { Boss Hog `json:"hog"` } -// +k8s:deepcopy-gen=false -// +k8s:openapi-gen=false -type Hog struct { - // Should be in status but not spec, since Hog isn't in DummySpec - // +operator-sdk:csv:customresourcedefinitions:type=spec - // +operator-sdk:csv:customresourcedefinitions:type=status,displayName="boss-hog-engine" - Engine Engine `json:"engine"` - // Not in spec or status, no boolean annotation - // +operator-sdk:csv:customresourcedefinitions:displayName="doesnt-matter" - Brand string `json:"brand"` - // Not in spec or status - Helmet string `json:"helmet"` - // Fields should be inlined - // +operator-sdk:csv:customresourcedefinitions:type=spec - // +operator-sdk:csv:customresourcedefinitions:type=status - Inlined InlinedComponent `json:",inline"` - // Fields should be inlined - InlinedComponent `json:",inline"` - // Should be ignored - // +operator-sdk:csv:customresourcedefinitions:type=spec - // +operator-sdk:csv:customresourcedefinitions:type=status - Ignored IgnoredComponent `json:"-"` - // Should be ignored, but exported children should not be - notExported `json:",inline"` -} - -type notExported struct { - // +operator-sdk:csv:customresourcedefinitions:type=spec - // +operator-sdk:csv:customresourcedefinitions:type=status - Public string `json:"foo"` - // +operator-sdk:csv:customresourcedefinitions:type=spec - // +operator-sdk:csv:customresourcedefinitions:type=status - private string `json:"-"` -} - -// +k8s:deepcopy-gen=false -// +k8s:openapi-gen=false -type Engine struct { - // Should not be included, no annotations. - Pistons []string `json:"pistons"` -} - -// +k8s:deepcopy-gen=false -// +k8s:openapi-gen=false -type Wheel struct { - // Type should be in spec with path equal to wheels[0].type - // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Wheel Type",xDescriptors="urn:alm:descriptor:com.tectonic.ui:arrayFieldGroup:wheels" ; "urn:alm:descriptor:com.tectonic.ui:text" - Type string `json:"type"` -} - -// +k8s:deepcopy-gen=false -// +k8s:openapi-gen=false -type InlinedComponent struct { - // +operator-sdk:csv:customresourcedefinitions:type=spec - // +operator-sdk:csv:customresourcedefinitions:type=status - SeatMaterial string `json:"seatMaterial"` -} - -// +k8s:deepcopy-gen=false -// +k8s:openapi-gen=false -type IgnoredComponent struct { - // +operator-sdk:csv:customresourcedefinitions:type=spec - // +operator-sdk:csv:customresourcedefinitions:type=status - TrunkSpace string `json:"trunkSpace"` -} - // +k8s:deepcopy-gen=false // +k8s:openapi-gen=false type OtherDummyStatus struct { diff --git a/internal/generate/testdata/go/api/v1alpha2/motorcycle_types.go b/internal/generate/testdata/go/api/v1alpha2/motorcycle_types.go new file mode 100644 index 0000000000..b611afe9e1 --- /dev/null +++ b/internal/generate/testdata/go/api/v1alpha2/motorcycle_types.go @@ -0,0 +1,81 @@ +// Copyright 2021 The Operator-SDK Authors +// +// 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 v1alpha2 + +// +k8s:deepcopy-gen=false +// +k8s:openapi-gen=false +type Hog struct { + // Should be in status but not spec, since Hog isn't in DummySpec + // +operator-sdk:csv:customresourcedefinitions:type=spec + // +operator-sdk:csv:customresourcedefinitions:type=status,displayName="boss-hog-engine" + Engine Engine `json:"engine"` + // Not in spec or status, no boolean annotation + // +operator-sdk:csv:customresourcedefinitions:displayName="doesnt-matter" + Brand string `json:"brand"` + // Not in spec or status + Helmet string `json:"helmet"` + // Fields should be inlined + // +operator-sdk:csv:customresourcedefinitions:type=spec + // +operator-sdk:csv:customresourcedefinitions:type=status + Inlined InlinedComponent `json:",inline"` + // Fields should be inlined + InlinedComponent `json:",inline"` + // Should be ignored + // +operator-sdk:csv:customresourcedefinitions:type=spec + // +operator-sdk:csv:customresourcedefinitions:type=status + Ignored IgnoredComponent `json:"-"` + // Should be ignored, but exported children should not be + notExported `json:",inline"` +} + +type notExported struct { + // +operator-sdk:csv:customresourcedefinitions:type=spec + // +operator-sdk:csv:customresourcedefinitions:type=status + Public string `json:"foo"` + // +operator-sdk:csv:customresourcedefinitions:type=spec + // +operator-sdk:csv:customresourcedefinitions:type=status + private string `json:"-"` +} + +// +k8s:deepcopy-gen=false +// +k8s:openapi-gen=false +type Engine struct { + // Should not be included, no annotations. + Pistons []string `json:"pistons"` +} + +// +k8s:deepcopy-gen=false +// +k8s:openapi-gen=false +type Wheel struct { + // Type should be in spec with path equal to wheels[0].type + // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Wheel Type",xDescriptors="urn:alm:descriptor:com.tectonic.ui:arrayFieldGroup:wheels" ; "urn:alm:descriptor:com.tectonic.ui:text" + Type string `json:"type"` +} + +// +k8s:deepcopy-gen=false +// +k8s:openapi-gen=false +type InlinedComponent struct { + // +operator-sdk:csv:customresourcedefinitions:type=spec + // +operator-sdk:csv:customresourcedefinitions:type=status + SeatMaterial string `json:"seatMaterial"` +} + +// +k8s:deepcopy-gen=false +// +k8s:openapi-gen=false +type IgnoredComponent struct { + // +operator-sdk:csv:customresourcedefinitions:type=spec + // +operator-sdk:csv:customresourcedefinitions:type=status + TrunkSpace string `json:"trunkSpace"` +}