diff --git a/changelog/fragments/csv-gen-package-aware.yaml b/changelog/fragments/csv-gen-package-aware.yaml new file mode 100644 index 0000000000..85715ed46a --- /dev/null +++ b/changelog/fragments/csv-gen-package-aware.yaml @@ -0,0 +1,6 @@ +entries: + - description: > + For Go-based projects, `generate ` subcommands now consider package and type names + when parsing Go API types files to generate a CSV's `owned.customresourcedefinitions`, such that types in + different packages and files will not overwrite each other. + kind: bugfix diff --git a/internal/generate/clusterserviceversion/bases/definitions/ast.go b/internal/generate/clusterserviceversion/bases/definitions/ast.go index bdfad9fb42..b3717b6799 100644 --- a/internal/generate/clusterserviceversion/bases/definitions/ast.go +++ b/internal/generate/clusterserviceversion/bases/definitions/ast.go @@ -18,15 +18,90 @@ import ( "errors" "fmt" "go/ast" + "strconv" "strings" + "sigs.k8s.io/controller-tools/pkg/crd" + "sigs.k8s.io/controller-tools/pkg/loader" "sigs.k8s.io/controller-tools/pkg/markers" ) -// getMarkedChildrenOfField collects all marked fields from type declarations starting at root in depth-first order. -func (g generator) getMarkedChildrenOfField(root markers.FieldInfo) (map[string][]*fieldInfo, error) { +// importIdents maps import identifiers to a list of corresponding path and file containing that path. +// For example, consider the set of 2 files, one containing 'import foo "my/foo"' and the other 'import foo "your/foo"'. +// Then the map would be: map["foo"][]struct{{f: (file 1), path: "my/foo"},{f: (file 2), path: "your/foo"}}. +type importIdents map[string][]struct { + f *ast.File + path string +} + +// newImportIdents creates an importIdents from all imports in pkg. +func newImportIdents(pkg *loader.Package) (importIdents, error) { + importIDs := make(map[string][]struct { + f *ast.File + path string + }) + for _, file := range pkg.Syntax { + for _, impSpec := range file.Imports { + val, err := strconv.Unquote(impSpec.Path.Value) + if err != nil { + return nil, err + } + // Most imports are not locally named, so the real package name should be used. + var impName string + if imp, hasImp := pkg.Imports()[val]; hasImp { + impName = imp.Name + } + // impSpec.Name will not be empty for locally named imports + if impSpec.Name != nil { + impName = impSpec.Name.Name + } + importIDs[impName] = append(importIDs[impName], struct { + f *ast.File + path string + }{file, val}) + } + } + return importIDs, nil +} + +// findPackagePathForSelExpr returns the package path corresponding to the package name used in expr if it exists in im. +func (im importIdents) findPackagePathForSelExpr(expr *ast.SelectorExpr) (pkgPath string) { + // X contains the name being selected from. + xIdent, isIdent := expr.X.(*ast.Ident) + if !isIdent { + return "" + } + // Imports for all import statements where local import name == name being selected from. + imports, hasImports := im[xIdent.String()] + if !hasImports { + return "" + } + + // 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 + } + } + return "" +} + +// 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) { + // Gather all types and imports needed to build the BFS tree. + rootPkg.NeedTypesInfo() + importIDs, err := newImportIdents(rootPkg) + if err != nil { + return nil, err + } + // ast.Inspect will not traverse into fields, so iteratively collect them and to check for markers. - nextFields := []*fieldInfo{{FieldInfo: root}} + nextFields := []*fieldInfo{{FieldInfo: rootField}} markedFields := map[string][]*fieldInfo{} for len(nextFields) > 0 { fields := []*fieldInfo{} @@ -36,49 +111,65 @@ func (g generator) getMarkedChildrenOfField(root markers.FieldInfo) (map[string] if n == nil { return true } - switch expr := n.(type) { + + var info *markers.TypeInfo + var hasInfo bool + switch nt := n.(type) { + case *ast.SelectorExpr: + // Case of a 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}] + } case *ast.Ident: + // Case of a local type definition. + // Only look at type names. - if expr.Obj == nil || expr.Obj.Kind != ast.Typ { - return true + 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}] } - // Check if the field's type exists in the known types. - info, hasInfo := g.types[expr.Name] - if !hasInfo { + } + if !hasInfo { + return true + } + + // Add all child fields to the list to search next. + 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 } - // Add all child fields to the list to search next. - 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 - } - // Add extra information to the segment if it comes from a certain field type. - switch finfo.RawField.Type.(type) { - case (*ast.ArrayType): - // arrayFieldGroup case. - if segment != ignoredTag && segment != inlinedTag { - segment += "[0]" - } - } - // Create a new set of path segments using the parent's segments - // and add the field to the next fields to search. - parentSegments := make([]string, len(field.pathSegments), len(field.pathSegments)+1) - copy(parentSegments, field.pathSegments) - f := &fieldInfo{ - FieldInfo: finfo, - pathSegments: append(parentSegments, segment), - } - 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) + // Add extra information to the segment if it comes from a certain field type. + switch finfo.RawField.Type.(type) { + case *ast.ArrayType: + // arrayFieldGroup case. + if segment != ignoredTag && segment != inlinedTag { + segment += "[0]" } } + // Create a new set of path segments using the parent's segments + // and add the field to the next fields to search. + parentSegments := make([]string, len(field.pathSegments), len(field.pathSegments)+1) + copy(parentSegments, field.pathSegments) + f := &fieldInfo{ + FieldInfo: finfo, + pathSegments: append(parentSegments, segment), + } + 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) + } } + return true }) if err := fmtParseErrors(errs); err != nil { diff --git a/internal/generate/clusterserviceversion/bases/definitions/crd.go b/internal/generate/clusterserviceversion/bases/definitions/crd.go index 5167b79095..0f0c01036f 100644 --- a/internal/generate/clusterserviceversion/bases/definitions/crd.go +++ b/internal/generate/clusterserviceversion/bases/definitions/crd.go @@ -26,7 +26,9 @@ import ( "github.com/operator-framework/api/pkg/operators/v1alpha1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/version" + "sigs.k8s.io/controller-tools/pkg/crd" crdmarkers "sigs.k8s.io/controller-tools/pkg/crd/markers" + "sigs.k8s.io/controller-tools/pkg/loader" "sigs.k8s.io/controller-tools/pkg/markers" "github.com/operator-framework/operator-sdk/internal/util/k8sutil" @@ -54,7 +56,7 @@ func getHalfBySep(s, sep string, half uint) string { // buildCRDDescriptionFromType builds a crdDescription for the Go API defined // by key from markers and type information in g.types. -func (g generator) buildCRDDescriptionFromType(gvk schema.GroupVersionKind, kindType *markers.TypeInfo) (v1alpha1.CRDDescription, int, error) { +func (g generator) buildCRDDescriptionFromType(gvk schema.GroupVersionKind, typeIdent crd.TypeIdent, kindType *markers.TypeInfo) (v1alpha1.CRDDescription, int, error) { // Initialize the description. description := v1alpha1.CRDDescription{ @@ -96,7 +98,7 @@ func (g generator) buildCRDDescriptionFromType(gvk schema.GroupVersionKind, kind } sortResources(description.Resources) - specDescriptors, err := g.getTypedDescriptors(kindType, reflect.TypeOf(v1alpha1.SpecDescriptor{}), spec) + specDescriptors, err := g.getTypedDescriptors(typeIdent.Package, kindType, reflect.TypeOf(v1alpha1.SpecDescriptor{}), spec) if err != nil { return v1alpha1.CRDDescription{}, 0, err } @@ -104,7 +106,7 @@ func (g generator) buildCRDDescriptionFromType(gvk schema.GroupVersionKind, kind description.SpecDescriptors = append(description.SpecDescriptors, d.(v1alpha1.SpecDescriptor)) } - statusDescriptors, err := g.getTypedDescriptors(kindType, reflect.TypeOf(v1alpha1.StatusDescriptor{}), status) + statusDescriptors, err := g.getTypedDescriptors(typeIdent.Package, kindType, reflect.TypeOf(v1alpha1.StatusDescriptor{}), status) if err != nil { return v1alpha1.CRDDescription{}, 0, err } @@ -115,7 +117,7 @@ func (g generator) buildCRDDescriptionFromType(gvk schema.GroupVersionKind, kind return description, descriptionOrder, nil } -func (g generator) getTypedDescriptors(kindType *markers.TypeInfo, t reflect.Type, descType string) ([]interface{}, error) { +func (g generator) getTypedDescriptors(pkg *loader.Package, kindType *markers.TypeInfo, t reflect.Type, descType string) ([]interface{}, error) { // Find child in the kind type. child, err := findChildForDescType(kindType, descType) if err != nil { @@ -123,7 +125,7 @@ func (g generator) getTypedDescriptors(kindType *markers.TypeInfo, t reflect.Typ } // Find annotated fields of child and parse them into descriptors. - markedFields, err := g.getMarkedChildrenOfField(child) + markedFields, err := g.getMarkedChildrenOfField(pkg, child) if err != nil { return nil, err } diff --git a/internal/generate/clusterserviceversion/bases/definitions/definitions.go b/internal/generate/clusterserviceversion/bases/definitions/definitions.go index 7efd5b471b..a51855b78c 100644 --- a/internal/generate/clusterserviceversion/bases/definitions/definitions.go +++ b/internal/generate/clusterserviceversion/bases/definitions/definitions.go @@ -24,6 +24,8 @@ import ( log "github.com/sirupsen/logrus" "golang.org/x/tools/go/packages" "k8s.io/apimachinery/pkg/runtime/schema" + apiversion "k8s.io/apimachinery/pkg/version" + "sigs.k8s.io/controller-tools/pkg/crd" "sigs.k8s.io/controller-tools/pkg/genall" "sigs.k8s.io/controller-tools/pkg/loader" "sigs.k8s.io/controller-tools/pkg/markers" @@ -39,77 +41,60 @@ type descriptionValues struct { // to populate csv spec fields. Go code with relevant markers and information is expected to be // in a package under apisRootDir and match a GVK in keys. func ApplyDefinitionsForKeysGo(csv *v1alpha1.ClusterServiceVersion, apisRootDir string, gvks []schema.GroupVersionKind) error { - - // Construct a set of probable paths under apisRootDir for types defined by gvks. - // These are usually '(pkg/)?apis/(/)?'. - // NB(estroz): using "leaf" packages prevents type builders from searching other packages. - // It would be nice to implement extra-package traversal in the future. - paths, err := makeAPIPaths(apisRootDir, gvks) + wd, err := os.Getwd() if err != nil { return err } - // Some APIs may not exist under apisRootDir, so skip loading packages if no paths are found - if len(paths) == 0 { - return nil - } - // Collect Go types from roots. + // Create a generator context for type-checking and loading all packages under apisRootDir. + // The "$(pwd)//..." syntax directs the loader to load all packages under apisRootDir. g := &generator{} - ctx, err := g.contextForRoots(paths...) + ctx, err := g.contextForRoots(filepath.Join(wd, apisRootDir) + "/...") if err != nil { return err } + // Collect Go types from the API root. g.needTypes(ctx) if loader.PrintErrors(ctx.Roots, packages.TypeError) { return errors.New("one or more API packages had type errors") } + gvkSet := make(map[schema.GroupVersionKind]struct{}, len(gvks)) + for _, gvk := range gvks { + gvkSet[gvk] = struct{}{} + } + // Create definitions for kind types found under the collected roots. definitionsByGVK := make(map[schema.GroupVersionKind]*descriptionValues) - for _, gvk := range gvks { - kindType, hasKind := g.types[gvk.Kind] - if !hasKind { - log.Warnf("Skipping CSV annotation parsing for API %s: type %s not found", gvk, gvk.Kind) - continue - } - crd, crdOrder, err := g.buildCRDDescriptionFromType(gvk, kindType) - if err != nil { - return err - } - definitionsByGVK[gvk] = &descriptionValues{ - crdOrder: crdOrder, - crd: crd, + for typeIdent, typeInfo := range g.types { + if gv, hasGV := g.groupVersions[typeIdent.Package]; hasGV { + gvk := gv.WithKind(typeIdent.Name) + // Type is one of the GVKs specified by the caller. + if _, hasGVK := gvkSet[gvk]; hasGVK { + crd, crdOrder, err := g.buildCRDDescriptionFromType(gvk, typeIdent, typeInfo) + if err != nil { + return err + } + definitionsByGVK[gvk] = &descriptionValues{ + crdOrder: crdOrder, + crd: crd, + } + delete(gvkSet, gvk) + } } } + // Leftover GVKs are ignored because their types can't be found. + for _, gvk := range gvkSet { + log.Warnf("Skipping CSV annotation parsing for API %s: type not found", gvk) + } + // Update csv with all values parsed. updateDefinitionsByKey(csv, definitionsByGVK) return nil } -// makeAPIPaths creates a set of API directory paths with apisRootDir as their parent. -func makeAPIPaths(apisRootDir string, gvks []schema.GroupVersionKind) (paths []string, err error) { - if apisRootDir, err = filepath.Abs(apisRootDir); err != nil { - return nil, err - } - - for _, gvk := range gvks { - // Check if the kind pkg is at the expected layout. - group := MakeGroupFromFullGroup(gvk.Group) - expectedPkgPath, err := getExpectedPkgLayout(apisRootDir, group, gvk.Version) - if err != nil { - return nil, err - } - if expectedPkgPath == "" { - log.Warnf("Skipping CSV annotation parsing for API %s: directory does not exist", gvk) - continue - } - paths = append(paths, expectedPkgPath) - } - return paths, nil -} - // updateDefinitionsByKey updates owned definitions that already exist in csv or adds new definitions that do not. func updateDefinitionsByKey(csv *v1alpha1.ClusterServiceVersion, defsByGVK map[schema.GroupVersionKind]*descriptionValues) { // Create a set of buckets for all generated descriptions. @@ -122,9 +107,7 @@ func updateDefinitionsByKey(csv *v1alpha1.ClusterServiceVersion, defsByGVK map[s // Sort generated buckets before adding non-generated descriptions so users can // set their order manually. for _, bucket := range crdBuckets { - sort.Slice(bucket, func(i, j int) bool { - return bucket[i].Name < bucket[j].Name - }) + sort.Slice(bucket, lessForCRDDescription(bucket)) } // Append non-generated descriptions to the end of their buckets, @@ -149,6 +132,19 @@ func updateDefinitionsByKey(csv *v1alpha1.ClusterServiceVersion, defsByGVK map[s } } +// lessForCRDDescription returns a less func for descs. Used for sorting a list of CRDDescriptions. +func lessForCRDDescription(descs []v1alpha1.CRDDescription) func(i, j int) bool { + return func(i, j int) bool { + if descs[i].Name == descs[j].Name { + if descs[i].Kind == descs[j].Kind { + return apiversion.CompareKubeAwareVersionStrings(descs[i].Version, descs[j].Version) > 0 + } + return descs[i].Kind < descs[j].Kind + } + return descs[i].Name < descs[j].Name + } +} + // descToGVK convert desc to a GVK type. func descToGVK(desc v1alpha1.CRDDescription) (gvk schema.GroupVersionKind) { gvk.Group = MakeFullGroupFromName(desc.Name) @@ -157,47 +153,10 @@ func descToGVK(desc v1alpha1.CRDDescription) (gvk schema.GroupVersionKind) { return gvk } -func isDirExist(path string) (bool, error) { - fileInfo, err := os.Stat(path) - if err != nil { - if os.IsNotExist(err) { - return false, nil - } - return false, err - } - return fileInfo.IsDir(), nil -} - -// getExpectedPkgLayout checks the directory layout in apisRootDir for single and multi group layouts and returns -// the expected pkg path for the group and version. Returns empty string if neither single or multi group layout -// is detected. -// - multi-group layout: apis// -// - single-group layout: api/ -func getExpectedPkgLayout(apisRootDir, group, version string) (expectedPkgPath string, err error) { - if group == "" || version == "" { - return "", nil - } - groupVersionDir := filepath.Join(apisRootDir, group, version) - if isMultiGroupLayout, err := isDirExist(groupVersionDir); isMultiGroupLayout { - if err != nil { - return "", err - } - return groupVersionDir, nil - } - versionDir := filepath.Join(apisRootDir, version) - if isSingleGroupLayout, err := isDirExist(versionDir); isSingleGroupLayout { - if err != nil { - return "", err - } - return versionDir, nil - } - // Neither multi nor single group layout - return "", nil -} - // generator creates API definitions from type information for a set of roots. type generator struct { - types map[string]*markers.TypeInfo + types map[crd.TypeIdent]*markers.TypeInfo + groupVersions map[*loader.Package]schema.GroupVersion } // contextForRoots creates a context that can populate a generator for a set of roots loaded from dirs. @@ -222,13 +181,41 @@ func (g *generator) contextForRoots(dirs ...string) (ctx *genall.GenerationConte } // needTypes sets types in the generator for a given context. +// Adapted from https://github.com/kubernetes-sigs/controller-tools/blob/868d39a/pkg/crd/parser.go#L121 func (g *generator) needTypes(ctx *genall.GenerationContext) { - g.types = make(map[string]*markers.TypeInfo) - cb := func(info *markers.TypeInfo) { - g.types[info.Name] = info - } + g.types = make(map[crd.TypeIdent]*markers.TypeInfo) + g.groupVersions = make(map[*loader.Package]schema.GroupVersion) for _, root := range ctx.Roots { - if err := markers.EachType(ctx.Collector, root, cb); err != nil { + pkgMarkers, err := markers.PackageMarkers(ctx.Collector, root) + if err != nil { + root.AddError(err) + } else { + // Explicitly skip this package. + if skipPkg := pkgMarkers.Get("kubebuilder:skip"); skipPkg != nil { + return + } + // Get group name and optionall version name from package markers. + if nameVal := pkgMarkers.Get("groupName"); nameVal != nil { + versionVal := root.Name + if versionMarker := pkgMarkers.Get("versionName"); versionMarker != nil { + versionVal = versionMarker.(string) + } + + g.groupVersions[root] = schema.GroupVersion{ + Version: versionVal, + Group: nameVal.(string), + } + } + } + // Add all types indexed by their package and type name. + f := func(info *markers.TypeInfo) { + ident := crd.TypeIdent{ + Package: root, + Name: info.Name, + } + g.types[ident] = info + } + if err := markers.EachType(ctx.Collector, root, f); err != nil { root.AddError(err) } } diff --git a/internal/generate/clusterserviceversion/bases/definitions/definitions_test.go b/internal/generate/clusterserviceversion/bases/definitions/definitions_test.go index c1f7c689c9..7828dfea2f 100644 --- a/internal/generate/clusterserviceversion/bases/definitions/definitions_test.go +++ b/internal/generate/clusterserviceversion/bases/definitions/definitions_test.go @@ -78,8 +78,10 @@ func TestApplyDefinitionsForKeysGo(t *testing.T) { {Name: "dummy-replicaset", Kind: "ReplicaSet", Version: "v1beta2"}, }, SpecDescriptors: []v1alpha1.SpecDescriptor{ + {Path: "sideCar", DisplayName: "Side Car"}, {Path: "size", DisplayName: "dummy-size", Description: "Should be in spec", XDescriptors: []string{"urn:alm:descriptor:com.tectonic.ui:podCount"}}, + {Path: "useful.containers", DisplayName: "Containers"}, {Path: "wheels", DisplayName: "Wheels", Description: "Should be in spec, but should not have array index in path", XDescriptors: []string{"urn:alm:descriptor:com.tectonic.ui:text"}}, @@ -134,47 +136,58 @@ func TestApplyDefinitionsForKeysGo(t *testing.T) { }, }, { - description: "Do not change definitions with non-existent package dir", - apisDir: filepath.Join("pkg", "notexist"), - csv: &v1alpha1.ClusterServiceVersion{ - Spec: v1alpha1.ClusterServiceVersionSpec{ - CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{ - Owned: []v1alpha1.CRDDescription{ - { - Name: "dummys.cache.example.com", Version: "v1alpha2", Kind: "Dummy", - DisplayName: "Dummy App", - Description: "Dummy is the Schema for the other dummy API", - Resources: []v1alpha1.APIResourceReference{ - {Name: "dummy-pod", Kind: "Pod", Version: "v1"}, - }, - SpecDescriptors: []v1alpha1.SpecDescriptor{ - {Path: "foo", DisplayName: "Foo", Description: "Should not be removed"}, - }, - StatusDescriptors: []v1alpha1.StatusDescriptor{ - {Path: "bar", DisplayName: "Bar", Description: "Should not be removed"}, - }, - }, - }, - }, - }, - }, + description: "Populate CRDDescription with GVKs with same GK and different versions", + apisDir: "api", + csv: &v1alpha1.ClusterServiceVersion{}, gvks: []schema.GroupVersionKind{ - {Group: "cache.example.com", Version: "v1alpha2", Kind: "Dummy"}, + {Group: "cache.example.com", Version: "v1alpha1", Kind: "Memcached"}, + {Group: "cache.example.com", Version: "v1alpha2", Kind: "Memcached"}, }, expectedCRDs: v1alpha1.CustomResourceDefinitions{ Owned: []v1alpha1.CRDDescription{ { - Name: "dummys.cache.example.com", Version: "v1alpha2", Kind: "Dummy", - DisplayName: "Dummy App", - Description: "Dummy is the Schema for the other dummy API", - Resources: []v1alpha1.APIResourceReference{ - {Name: "dummy-pod", Kind: "Pod", Version: "v1"}, - }, + Name: "memcacheds.cache.example.com", Version: "v1alpha2", Kind: "Memcached", + DisplayName: "Memcached App", + Description: "Memcached is the Schema for the memcacheds API", SpecDescriptors: []v1alpha1.SpecDescriptor{ - {Path: "foo", DisplayName: "Foo", Description: "Should not be removed"}, + {Path: "size", DisplayName: "Size", Description: "Size is the size of the memcached deployment"}, }, StatusDescriptors: []v1alpha1.StatusDescriptor{ - {Path: "bar", DisplayName: "Bar", Description: "Should not be removed"}, + {Path: "nodes", DisplayName: "Nodes", Description: "Nodes are the names of the memcached pods"}, + }, + }, + { + Name: "memcacheds.cache.example.com", Version: "v1alpha1", Kind: "Memcached", + DisplayName: "Memcached App Display Name", + Description: "Memcached is the Schema for the memcacheds API", + StatusDescriptors: []v1alpha1.StatusDescriptor{ + {Path: "nodes", DisplayName: "Nodes", Description: "Nodes are the names of the memcached pods"}, + }, + SpecDescriptors: []v1alpha1.SpecDescriptor{ + {Path: "containers", DisplayName: "Containers"}, + {Path: "providers", DisplayName: "Providers", Description: "List of Providers"}, + {Path: "providers[0].foo", DisplayName: "Foo Provider", Description: "Foo represents the Foo provider"}, + {Path: "providers[0].foo.credentialsSecret", DisplayName: "Secret Containing the Credentials", + Description: "CredentialsSecret is a reference to a secret containing authentication details for the Foo server", + XDescriptors: []string{"urn:alm:descriptor:io.kubernetes:Secret"}, + }, + { + Path: "providers[0].foo.credentialsSecret.key", DisplayName: "Key within the secret", + Description: "Key represents the specific key to reference from the secret", + XDescriptors: []string{"urn:alm:descriptor:com.tectonic.ui:advanced", "urn:alm:descriptor:com.tectonic.ui:text"}, + }, + { + Path: "providers[0].foo.credentialsSecret.name", DisplayName: "Name of the secret", + Description: "Name represents the name of the secret", + XDescriptors: []string{"urn:alm:descriptor:com.tectonic.ui:advanced", "urn:alm:descriptor:com.tectonic.ui:text"}, + }, + { + Path: "providers[0].foo.credentialsSecret.namespace", DisplayName: "Namespace containing the secret", + Description: "Namespace represents the namespace containing the secret", + XDescriptors: []string{"urn:alm:descriptor:com.tectonic.ui:advanced", "urn:alm:descriptor:com.tectonic.ui:text"}, + }, + { + Path: "size", DisplayName: "Size", Description: "Size is the size of the memcached deployment"}, }, }, }, @@ -227,6 +240,15 @@ func TestApplyDefinitionsForKeysGo(t *testing.T) { }, }, }, + { + description: "Return error for non-existent package dir", + apisDir: filepath.Join("pkg", "notexist"), + csv: &v1alpha1.ClusterServiceVersion{}, + gvks: []schema.GroupVersionKind{ + {Group: "cache.example.com", Version: "v1alpha2", Kind: "Dummy"}, + }, + wantErr: true, + }, } for _, c := range cases { diff --git a/internal/generate/clusterserviceversion/bases/definitions/markers.go b/internal/generate/clusterserviceversion/bases/definitions/markers.go index 4865631d0b..7bdb24a5c2 100644 --- a/internal/generate/clusterserviceversion/bases/definitions/markers.go +++ b/internal/generate/clusterserviceversion/bases/definitions/markers.go @@ -37,17 +37,20 @@ const ( crdMarkerName = csvPrefix + ":customresourcedefinitions" ) -// +operator-sdk:csv:customresourcedefinitions:displayName="string",resources={ {kind,version,name} , ... } -var typeDefinition = markers.Must(markers.MakeDefinition(crdMarkerName, markers.DescribesType, Description{})) - -// +operator-sdk:csv:customresourcedefinitions:type=,displayName="name",xDescriptors="ui:elements:foo:bar" -var fieldDefinition = markers.Must(markers.MakeDefinition(crdMarkerName, markers.DescribesField, Descriptor{})) - -// See https://github.com/kubernetes-sigs/controller-tools/blob/92e95c1/pkg/crd/markers/crd.go#L40 -var crdResourceDefinition = markers.Must(markers.MakeDefinition("kubebuilder:resource", markers.DescribesType, crdmarkers.Resource{})) +var ( + // +operator-sdk:csv:customresourcedefinitions:displayName="string",resources={ {kind,version,name} , ... } + typeDefinition = markers.Must(markers.MakeDefinition(crdMarkerName, markers.DescribesType, Description{})) + // +operator-sdk:csv:customresourcedefinitions:type=,displayName="name",xDescriptors="ui:elements:foo:bar" + fieldDefinition = markers.Must(markers.MakeDefinition(crdMarkerName, markers.DescribesField, Descriptor{})) +) // registerMarkers adds type and field marker definitions to a registry. func registerMarkers(into *markers.Registry) error { + // External definitions. + if err := crdmarkers.Register(into); err != nil { + return fmt.Errorf("error registering external marker definition: %v", err) + } + if err := into.Register(typeDefinition); err != nil { return fmt.Errorf("error registering type definition: %v", err) } @@ -57,11 +60,6 @@ func registerMarkers(into *markers.Registry) error { } into.AddHelp(fieldDefinition, Descriptor{}.Help()) - // External definitions. - if err := into.Register(crdResourceDefinition); err != nil { - return fmt.Errorf("error registering CRD resource definition: %v", err) - } - into.AddHelp(crdResourceDefinition, crdmarkers.Resource{}.Help()) return nil } diff --git a/internal/generate/clusterserviceversion/clusterserviceversion_updaters.go b/internal/generate/clusterserviceversion/clusterserviceversion_updaters.go index faf8911686..233bb3484f 100644 --- a/internal/generate/clusterserviceversion/clusterserviceversion_updaters.go +++ b/internal/generate/clusterserviceversion/clusterserviceversion_updaters.go @@ -18,7 +18,6 @@ import ( "encoding/json" "errors" "fmt" - "sort" "strings" operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" @@ -29,7 +28,6 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" - "k8s.io/apimachinery/pkg/version" "github.com/operator-framework/operator-sdk/internal/generate/collector" "github.com/operator-framework/operator-sdk/internal/util/k8sutil" @@ -46,9 +44,6 @@ func ApplyTo(c *collector.Manifests, csv *operatorsv1alpha1.ClusterServiceVersio // Set fields required by namespaced operators. This is a no-op for cluster-scoped operators. setNamespacedFields(csv) - // Sort all updated fields. - sortUpdates(csv) - return validate(csv) } @@ -474,30 +469,6 @@ func applyCustomResources(c *collector.Manifests, csv *operatorsv1alpha1.Cluster return nil } -// sortUpdates sorts all fields updated in csv. -// TODO(estroz): sort other modified fields. -func sortUpdates(csv *operatorsv1alpha1.ClusterServiceVersion) { - sort.Sort(descSorter(csv.Spec.CustomResourceDefinitions.Owned)) - sort.Sort(descSorter(csv.Spec.CustomResourceDefinitions.Required)) -} - -// descSorter sorts a set of crdDescriptions. -type descSorter []operatorsv1alpha1.CRDDescription - -var _ sort.Interface = descSorter{} - -func (descs descSorter) Len() int { return len(descs) } -func (descs descSorter) Less(i, j int) bool { - if descs[i].Name == descs[j].Name { - if descs[i].Kind == descs[j].Kind { - return version.CompareKubeAwareVersionStrings(descs[i].Version, descs[j].Version) > 0 - } - return descs[i].Kind < descs[j].Kind - } - return descs[i].Name < descs[j].Name -} -func (descs descSorter) Swap(i, j int) { descs[i], descs[j] = descs[j], descs[i] } - // validate will validate csv using the api validation library. // More info: https://github.com/operator-framework/api func validate(csv *operatorsv1alpha1.ClusterServiceVersion) error { diff --git a/internal/generate/testdata/go/api/shared/doc.go b/internal/generate/testdata/go/api/shared/doc.go new file mode 100644 index 0000000000..6a46e8b5d6 --- /dev/null +++ b/internal/generate/testdata/go/api/shared/doc.go @@ -0,0 +1,15 @@ +// 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 shared diff --git a/internal/generate/testdata/go/api/shared/memcached_types.go b/internal/generate/testdata/go/api/shared/memcached_types.go new file mode 100644 index 0000000000..8e61f8fa0c --- /dev/null +++ b/internal/generate/testdata/go/api/shared/memcached_types.go @@ -0,0 +1,25 @@ +// 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 shared + +import ( + v1 "k8s.io/api/core/v1" +) + +// UsefulType is a type shared between APIs. +type UsefulType struct { + // +operator-sdk:csv:customresourcedefinitions:type=spec + Containers []v1.Container `json:"containers"` +} diff --git a/internal/generate/testdata/go/api/v1alpha1/memcached_types.go b/internal/generate/testdata/go/api/v1alpha1/memcached_types.go index ea5b3d6dd3..d6bcb6ce4a 100644 --- a/internal/generate/testdata/go/api/v1alpha1/memcached_types.go +++ b/internal/generate/testdata/go/api/v1alpha1/memcached_types.go @@ -16,6 +16,8 @@ package v1alpha1 import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/operator-framework/operator-sdk/internal/generate/testdata/go/api/shared" ) // MemcachedSpec defines the desired state of Memcached @@ -27,6 +29,9 @@ type MemcachedSpec struct { // List of Providers // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Providers" Providers []Provider `json:"providers,omitempty"` + + // A useful shared type. + Useful shared.UsefulType `json:",inline"` } // Provider represents the container for a single provider diff --git a/internal/generate/testdata/go/api/v1alpha2/dummy_types.go b/internal/generate/testdata/go/api/v1alpha2/dummy_types.go index 9f70963c53..de32b5fe3f 100644 --- a/internal/generate/testdata/go/api/v1alpha2/dummy_types.go +++ b/internal/generate/testdata/go/api/v1alpha2/dummy_types.go @@ -15,7 +15,11 @@ package v1alpha2 import ( + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + // Has the same name as a different import in memcached_types.go to test duplicate package names. + foo "github.com/operator-framework/operator-sdk/internal/generate/testdata/go/api/shared" ) // +k8s:deepcopy-gen=false @@ -47,6 +51,10 @@ type DummySpec struct { // Should be in spec, but should not have array index in path // +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Wheels",xDescriptors="urn:alm:descriptor:com.tectonic.ui:text" Wheels []Wheel `json:"wheels"` + // A useful shared type. + Useful foo.UsefulType `json:"useful"` + // +operator-sdk:csv:customresourcedefinitions:type=spec + SideCar v1.Container `json:"sideCar"` } // +k8s:deepcopy-gen=false diff --git a/internal/generate/testdata/go/api/v1alpha2/memcached_types.go b/internal/generate/testdata/go/api/v1alpha2/memcached_types.go index 28c8588f9c..c26a3cb36a 100644 --- a/internal/generate/testdata/go/api/v1alpha2/memcached_types.go +++ b/internal/generate/testdata/go/api/v1alpha2/memcached_types.go @@ -15,7 +15,8 @@ package v1alpha2 import ( - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + // Has the same name as a different import in dummy_types.go to test duplicate package names. + foo "k8s.io/apimachinery/pkg/apis/meta/v1" ) // MemcachedSpec defines the desired state of Memcached @@ -40,8 +41,8 @@ type MemcachedStatus struct { // +kubebuilder:storageversion // +operator-sdk:csv:customresourcedefinitions:displayName="Memcached App" type Memcached struct { - metav1.TypeMeta `json:",inline"` - metav1.ObjectMeta `json:"metadata,omitempty"` + foo.TypeMeta `json:",inline"` + foo.ObjectMeta `json:"metadata,omitempty"` Spec MemcachedSpec `json:"spec,omitempty"` Status MemcachedStatus `json:"status,omitempty"` @@ -51,7 +52,7 @@ type Memcached struct { // MemcachedList contains a list of Memcached type MemcachedList struct { - metav1.TypeMeta `json:",inline"` - metav1.ListMeta `json:"metadata,omitempty"` - Items []Memcached `json:"items"` + foo.TypeMeta `json:",inline"` + foo.ListMeta `json:"metadata,omitempty"` + Items []Memcached `json:"items"` }