From a381d3547156971b0135be557c5b2973314186fc Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 1 Mar 2021 14:33:41 +1300 Subject: [PATCH 01/61] Move supporting types into subpackage --- .../codegen/pipeline_create_storage_types.go | 200 +---------------- .../codegen/storage/storage_type_factory.go | 203 ++++++++++++++++++ 2 files changed, 207 insertions(+), 196 deletions(-) create mode 100644 hack/generator/pkg/codegen/storage/storage_type_factory.go diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index 93769b9fe9f..cf4f9dc0578 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -7,7 +7,7 @@ package codegen import ( "context" - "fmt" + "github.com/Azure/k8s-infra/hack/generator/pkg/codegen/storage" "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" kerrors "k8s.io/apimachinery/pkg/util/errors" @@ -23,8 +23,8 @@ func createStorageTypes() PipelineStage { func(ctx context.Context, types astmodel.Types) (astmodel.Types, error) { storageTypes := make(astmodel.Types) - visitor := makeStorageTypesVisitor(types) - vc := makeStorageTypesVisitorContext() + visitor := storage.MakeStorageTypesVisitor(types) + vc := storage.MakeStorageTypesVisitorContext() var errs []error for _, d := range types { d := d @@ -45,7 +45,7 @@ func createStorageTypes() PipelineStage { continue } - finalDef := def.WithDescription(descriptionForStorageVariant(d)) + finalDef := def.WithDescription(storage.DescriptionForStorageVariant(d)) storageTypes[finalDef.Name()] = finalDef } @@ -59,195 +59,3 @@ func createStorageTypes() PipelineStage { return types, nil }) } - -// makeStorageTypesVisitor returns a TypeVisitor to do the creation of dedicated storage types -func makeStorageTypesVisitor(types astmodel.Types) astmodel.TypeVisitor { - factory := &StorageTypeFactory{ - types: types, - } - - visitor := astmodel.TypeVisitorBuilder{ - VisitValidatedType: factory.stripValidations, - VisitTypeName: factory.remapNames, - VisitObjectType: factory.convertObjects, - VisitResourceType: factory.convertResources, - VisitFlaggedType: factory.skipArmTypes, - }.Build() - - factory.visitor = visitor - factory.propertyConversions = []propertyConversion{ - factory.preserveKubernetesResourceStorageProperties, - factory.convertPropertiesForStorage, - } - - return visitor -} - -type StorageTypeFactory struct { - types astmodel.Types - propertyConversions []propertyConversion - visitor astmodel.TypeVisitor -} - -// A property conversion accepts a property definition and optionally applies a conversion to make -// the property suitable for use on a storage type. Conversions return nil if they decline to -// convert, deferring the conversion to another. -type propertyConversion = func(property *astmodel.PropertyDefinition, ctx StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) - -func (factory *StorageTypeFactory) stripValidations(this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { - // strip all type validations from storage types, - // act as if they do not exist - return this.Visit(v.ElementType(), ctx) -} - -func (factory *StorageTypeFactory) remapNames(_ *astmodel.TypeVisitor, name astmodel.TypeName, ctx interface{}) (astmodel.Type, error) { - visitorContext := ctx.(StorageTypesVisitorContext) - - // Resolve the type name to the actual referenced type - actualDefinition, actualDefinitionFound := factory.types[name] - - // Check for property specific handling - if visitorContext.property != nil && actualDefinitionFound { - if et, ok := actualDefinition.Type().(*astmodel.EnumType); ok { - // Property type refers to an enum, so we use the base type instead - return et.BaseType(), nil - } - } - - // Map the type name into our storage namespace - localRef, ok := name.PackageReference.AsLocalPackage() - if !ok { - return name, nil - } - - storageRef := astmodel.MakeStoragePackageReference(localRef) - visitedName := astmodel.MakeTypeName(storageRef, name.Name()) - return visitedName, nil -} - -func (factory *StorageTypeFactory) convertResources( - this *astmodel.TypeVisitor, - resource *astmodel.ResourceType, - ctx interface{}) (astmodel.Type, error) { - - // storage resource types do not need defaulter interface, they have no webhooks - return resource.WithoutInterface(astmodel.DefaulterInterfaceName), nil -} - -func (factory *StorageTypeFactory) convertObjects( - _ *astmodel.TypeVisitor, - object *astmodel.ObjectType, - ctx interface{}) (astmodel.Type, error) { - visitorContext := ctx.(StorageTypesVisitorContext) - objectContext := visitorContext.forObject(object) - - var errs []error - properties := object.Properties() - for i, prop := range properties { - p, err := factory.makeStorageProperty(prop, objectContext) - if err != nil { - errs = append(errs, err) - } else { - properties[i] = p - } - } - - if len(errs) > 0 { - err := kerrors.NewAggregate(errs) - return nil, err - } - - objectType := astmodel.NewObjectType().WithProperties(properties...) - return astmodel.StorageFlag.ApplyTo(objectType), nil -} - -// makeStorageProperty applies a conversion to make a variant of the property for use when -// serializing to storage -func (factory *StorageTypeFactory) makeStorageProperty( - prop *astmodel.PropertyDefinition, - objectContext StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { - for _, conv := range factory.propertyConversions { - p, err := conv(prop, objectContext.forProperty(prop)) - if err != nil { - // Something went wrong, return the error - return nil, err - } - if p != nil { - // We have the conversion we need, return it promptly - return p, nil - } - } - - return nil, fmt.Errorf("failed to find a conversion for property %v", prop.PropertyName()) -} - -// preserveKubernetesResourceStorageProperties preserves properties required by the KubernetesResource interface as they're always required -func (factory *StorageTypeFactory) preserveKubernetesResourceStorageProperties( - prop *astmodel.PropertyDefinition, - _ StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { - if astmodel.IsKubernetesResourceProperty(prop.PropertyName()) { - // Keep these unchanged - return prop, nil - } - - // No opinion, defer to another conversion - return nil, nil -} - -func (factory *StorageTypeFactory) convertPropertiesForStorage( - prop *astmodel.PropertyDefinition, - objectContext StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { - propertyType, err := factory.visitor.Visit(prop.PropertyType(), objectContext) - if err != nil { - return nil, err - } - - p := prop.WithType(propertyType). - MakeOptional(). - WithDescription("") - - return p, nil -} - -func (factory *StorageTypeFactory) skipArmTypes( - tv *astmodel.TypeVisitor, - flaggedType *astmodel.FlaggedType, - ctx interface{}) (astmodel.Type, error) { - if flaggedType.HasFlag(astmodel.ARMFlag) { - // We don't want to do anything with ARM types - return flaggedType, nil - } - - return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) -} - -func descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { - pkg := definition.Name().PackageReference.PackageName() - - result := []string{ - fmt.Sprintf("Storage version of %v.%v", pkg, definition.Name().Name()), - } - result = append(result, definition.Description()...) - - return result -} - -type StorageTypesVisitorContext struct { - object *astmodel.ObjectType - property *astmodel.PropertyDefinition -} - -func makeStorageTypesVisitorContext() StorageTypesVisitorContext { - return StorageTypesVisitorContext{} -} - -func (context StorageTypesVisitorContext) forObject(object *astmodel.ObjectType) StorageTypesVisitorContext { - context.object = object - context.property = nil - return context -} - -func (context StorageTypesVisitorContext) forProperty(property *astmodel.PropertyDefinition) StorageTypesVisitorContext { - context.property = property - return context -} diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go new file mode 100644 index 00000000000..e3dd7431d5f --- /dev/null +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -0,0 +1,203 @@ +/* + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT license. + */ + +package storage + +import ( + "fmt" + "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" + kerrors "k8s.io/apimachinery/pkg/util/errors" +) + +// makeStorageTypesVisitor returns a TypeVisitor to do the creation of dedicated storage types +func MakeStorageTypesVisitor(types astmodel.Types) astmodel.TypeVisitor { + factory := &StorageTypeFactory{ + types: types, + } + + result := astmodel.MakeTypeVisitor() + result.VisitValidatedType = factory.visitValidatedType + result.VisitTypeName = factory.visitTypeName + result.VisitObjectType = factory.visitObjectType + result.VisitResourceType = factory.visitResourceType + result.VisitFlaggedType = factory.visitFlaggedType + + factory.visitor = result + factory.propertyConversions = []propertyConversion{ + factory.preserveKubernetesResourceStorageProperties, + factory.convertPropertiesForStorage, + } + + return result +} + +type StorageTypeFactory struct { + types astmodel.Types + propertyConversions []propertyConversion + visitor astmodel.TypeVisitor +} + +// A property conversion accepts a property definition and optionally applies a conversion to make +// the property suitable for use on a storage type. Conversions return nil if they decline to +// convert, deferring the conversion to another. +type propertyConversion = func(property *astmodel.PropertyDefinition, ctx StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) + +func (factory *StorageTypeFactory) visitValidatedType(this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { + // strip all type validations from storage types, + // act as if they do not exist + return this.Visit(v.ElementType(), ctx) +} + +func (factory *StorageTypeFactory) visitTypeName(_ *astmodel.TypeVisitor, name astmodel.TypeName, ctx interface{}) (astmodel.Type, error) { + visitorContext := ctx.(StorageTypesVisitorContext) + + // Resolve the type name to the actual referenced type + actualDefinition, actualDefinitionFound := factory.types[name] + + // Check for property specific handling + if visitorContext.property != nil && actualDefinitionFound { + if et, ok := actualDefinition.Type().(*astmodel.EnumType); ok { + // Property type refers to an enum, so we use the base type instead + return et.BaseType(), nil + } + } + + // Map the type name into our storage namespace + localRef, ok := name.PackageReference.AsLocalPackage() + if !ok { + return name, nil + } + + storageRef := astmodel.MakeStoragePackageReference(localRef) + visitedName := astmodel.MakeTypeName(storageRef, name.Name()) + return visitedName, nil +} + +func (factory *StorageTypeFactory) visitResourceType( + this *astmodel.TypeVisitor, + resource *astmodel.ResourceType, + ctx interface{}) (astmodel.Type, error) { + + // storage resource types do not need defaulter interface, they have no webhooks + return resource.WithoutInterface(astmodel.DefaulterInterfaceName), nil +} + +func (factory *StorageTypeFactory) visitObjectType( + _ *astmodel.TypeVisitor, + object *astmodel.ObjectType, + ctx interface{}) (astmodel.Type, error) { + visitorContext := ctx.(StorageTypesVisitorContext) + objectContext := visitorContext.forObject(object) + + var errs []error + properties := object.Properties() + for i, prop := range properties { + p, err := factory.makeStorageProperty(prop, objectContext) + if err != nil { + errs = append(errs, err) + } else { + properties[i] = p + } + } + + if len(errs) > 0 { + err := kerrors.NewAggregate(errs) + return nil, err + } + + objectType := astmodel.NewObjectType().WithProperties(properties...) + return astmodel.StorageFlag.ApplyTo(objectType), nil +} + +// makeStorageProperty applies a conversion to make a variant of the property for use when +// serializing to storage +func (factory *StorageTypeFactory) makeStorageProperty( + prop *astmodel.PropertyDefinition, + objectContext StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { + for _, conv := range factory.propertyConversions { + p, err := conv(prop, objectContext.forProperty(prop)) + if err != nil { + // Something went wrong, return the error + return nil, err + } + if p != nil { + // We have the conversion we need, return it promptly + return p, nil + } + } + + return nil, fmt.Errorf("failed to find a conversion for property %v", prop.PropertyName()) +} + +// preserveKubernetesResourceStorageProperties preserves properties required by the KubernetesResource interface as they're always required +func (factory *StorageTypeFactory) preserveKubernetesResourceStorageProperties( + prop *astmodel.PropertyDefinition, + _ StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { + if astmodel.IsKubernetesResourceProperty(prop.PropertyName()) { + // Keep these unchanged + return prop, nil + } + + // No opinion, defer to another conversion + return nil, nil +} + +func (factory *StorageTypeFactory) convertPropertiesForStorage( + prop *astmodel.PropertyDefinition, + objectContext StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { + propertyType, err := factory.visitor.Visit(prop.PropertyType(), objectContext) + if err != nil { + return nil, err + } + + p := prop.WithType(propertyType). + MakeOptional(). + WithDescription("") + + return p, nil +} + +func (factory *StorageTypeFactory) visitFlaggedType( + tv *astmodel.TypeVisitor, + flaggedType *astmodel.FlaggedType, + ctx interface{}) (astmodel.Type, error) { + if flaggedType.HasFlag(astmodel.ArmFlag) { + // We don't want to do anything with ARM types + return flaggedType, nil + } + + return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) +} + +func DescriptionForStorageVariant(definition astmodel.TypeDefinition) []string { + pkg := definition.Name().PackageReference.PackageName() + + result := []string{ + fmt.Sprintf("Storage version of %v.%v", pkg, definition.Name().Name()), + } + result = append(result, definition.Description()...) + + return result +} + +type StorageTypesVisitorContext struct { + object *astmodel.ObjectType + property *astmodel.PropertyDefinition +} + +func MakeStorageTypesVisitorContext() StorageTypesVisitorContext { + return StorageTypesVisitorContext{} +} + +func (context StorageTypesVisitorContext) forObject(object *astmodel.ObjectType) StorageTypesVisitorContext { + context.object = object + context.property = nil + return context +} + +func (context StorageTypesVisitorContext) forProperty(property *astmodel.PropertyDefinition) StorageTypesVisitorContext { + context.property = property + return context +} From 40d064046ccc37c1dcf6c668d2d1f64765babd13 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 1 Mar 2021 14:48:33 +1300 Subject: [PATCH 02/61] Move type visitor creation onto StorageTypeFactory --- .../codegen/pipeline_create_storage_types.go | 8 +-- .../codegen/storage/storage_type_factory.go | 57 ++++++++++++------- 2 files changed, 40 insertions(+), 25 deletions(-) diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index cf4f9dc0578..2d9b9df1714 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -22,8 +22,8 @@ func createStorageTypes() PipelineStage { "Create storage versions of CRD types", func(ctx context.Context, types astmodel.Types) (astmodel.Types, error) { - storageTypes := make(astmodel.Types) - visitor := storage.MakeStorageTypesVisitor(types) + storageFactory := storage.NewStorageTypeFactory() + visitor := storageFactory.MakeStorageTypesVisitor() vc := storage.MakeStorageTypesVisitorContext() var errs []error for _, d := range types { @@ -46,7 +46,7 @@ func createStorageTypes() PipelineStage { } finalDef := def.WithDescription(storage.DescriptionForStorageVariant(d)) - storageTypes[finalDef.Name()] = finalDef + storageFactory.Add(finalDef) } if len(errs) > 0 { @@ -54,7 +54,7 @@ func createStorageTypes() PipelineStage { return nil, err } - types.AddTypes(storageTypes) + types.AddTypes(storageFactory.Types()) return types, nil }) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index e3dd7431d5f..666e635cea4 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -11,32 +11,47 @@ import ( kerrors "k8s.io/apimachinery/pkg/util/errors" ) -// makeStorageTypesVisitor returns a TypeVisitor to do the creation of dedicated storage types -func MakeStorageTypesVisitor(types astmodel.Types) astmodel.TypeVisitor { - factory := &StorageTypeFactory{ - types: types, +type StorageTypeFactory struct { + types astmodel.Types + propertyConversions []propertyConversion + visitor astmodel.TypeVisitor +} + +// NewStorageTypeFactory creates a new instance of StorageTypeFactory ready for use +func NewStorageTypeFactory() *StorageTypeFactory { + result := &StorageTypeFactory{ + types: make(astmodel.Types), } - result := astmodel.MakeTypeVisitor() - result.VisitValidatedType = factory.visitValidatedType - result.VisitTypeName = factory.visitTypeName - result.VisitObjectType = factory.visitObjectType - result.VisitResourceType = factory.visitResourceType - result.VisitFlaggedType = factory.visitFlaggedType - - factory.visitor = result - factory.propertyConversions = []propertyConversion{ - factory.preserveKubernetesResourceStorageProperties, - factory.convertPropertiesForStorage, + result.propertyConversions = []propertyConversion{ + result.preserveKubernetesResourceStorageProperties, + result.convertPropertiesForStorage, } return result } -type StorageTypeFactory struct { - types astmodel.Types - propertyConversions []propertyConversion - visitor astmodel.TypeVisitor +func (f *StorageTypeFactory) Add(def astmodel.TypeDefinition) { + f.types[def.Name()] = def +} + +func (f *StorageTypeFactory) Types() astmodel.Types { + return f.types +} + +// makeStorageTypesVisitor returns a TypeVisitor to do the creation of dedicated storage types +func (f *StorageTypeFactory) MakeStorageTypesVisitor() astmodel.TypeVisitor { + + result := astmodel.MakeTypeVisitor() + result.VisitValidatedType = f.visitValidatedType + result.VisitTypeName = f.visitTypeName + result.VisitObjectType = f.visitObjectType + result.VisitResourceType = f.visitResourceType + result.VisitFlaggedType = f.visitFlaggedType + + f.visitor = result + + return result } // A property conversion accepts a property definition and optionally applies a conversion to make @@ -76,9 +91,9 @@ func (factory *StorageTypeFactory) visitTypeName(_ *astmodel.TypeVisitor, name a } func (factory *StorageTypeFactory) visitResourceType( - this *astmodel.TypeVisitor, + _ *astmodel.TypeVisitor, resource *astmodel.ResourceType, - ctx interface{}) (astmodel.Type, error) { + _ interface{}) (astmodel.Type, error) { // storage resource types do not need defaulter interface, they have no webhooks return resource.WithoutInterface(astmodel.DefaulterInterfaceName), nil From 22322441ef2b629839e1586e8aae06ab30dbd5d9 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 1 Mar 2021 15:21:19 +1300 Subject: [PATCH 03/61] Remove StorageTypesVisitorContext --- .../codegen/storage/storage_type_factory.go | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 666e635cea4..ab5532200b7 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -196,23 +196,3 @@ func DescriptionForStorageVariant(definition astmodel.TypeDefinition) []string { return result } - -type StorageTypesVisitorContext struct { - object *astmodel.ObjectType - property *astmodel.PropertyDefinition -} - -func MakeStorageTypesVisitorContext() StorageTypesVisitorContext { - return StorageTypesVisitorContext{} -} - -func (context StorageTypesVisitorContext) forObject(object *astmodel.ObjectType) StorageTypesVisitorContext { - context.object = object - context.property = nil - return context -} - -func (context StorageTypesVisitorContext) forProperty(property *astmodel.PropertyDefinition) StorageTypesVisitorContext { - context.property = property - return context -} From 8e32767b660744cc80bcd1b2f7b76917d47f0d08 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 1 Mar 2021 16:10:57 +1300 Subject: [PATCH 04/61] Rename all receivers --- .../codegen/storage/storage_type_factory.go | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index ab5532200b7..9b4761b5cd8 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -59,17 +59,17 @@ func (f *StorageTypeFactory) MakeStorageTypesVisitor() astmodel.TypeVisitor { // convert, deferring the conversion to another. type propertyConversion = func(property *astmodel.PropertyDefinition, ctx StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) -func (factory *StorageTypeFactory) visitValidatedType(this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { +func (f *StorageTypeFactory) visitValidatedType(this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { // strip all type validations from storage types, // act as if they do not exist return this.Visit(v.ElementType(), ctx) } -func (factory *StorageTypeFactory) visitTypeName(_ *astmodel.TypeVisitor, name astmodel.TypeName, ctx interface{}) (astmodel.Type, error) { +func (f *StorageTypeFactory) visitTypeName(_ *astmodel.TypeVisitor, name astmodel.TypeName, ctx interface{}) (astmodel.Type, error) { visitorContext := ctx.(StorageTypesVisitorContext) // Resolve the type name to the actual referenced type - actualDefinition, actualDefinitionFound := factory.types[name] + actualDefinition, actualDefinitionFound := f.types[name] // Check for property specific handling if visitorContext.property != nil && actualDefinitionFound { @@ -90,7 +90,7 @@ func (factory *StorageTypeFactory) visitTypeName(_ *astmodel.TypeVisitor, name a return visitedName, nil } -func (factory *StorageTypeFactory) visitResourceType( +func (f *StorageTypeFactory) visitResourceType( _ *astmodel.TypeVisitor, resource *astmodel.ResourceType, _ interface{}) (astmodel.Type, error) { @@ -99,7 +99,7 @@ func (factory *StorageTypeFactory) visitResourceType( return resource.WithoutInterface(astmodel.DefaulterInterfaceName), nil } -func (factory *StorageTypeFactory) visitObjectType( +func (f *StorageTypeFactory) visitObjectType( _ *astmodel.TypeVisitor, object *astmodel.ObjectType, ctx interface{}) (astmodel.Type, error) { @@ -109,7 +109,7 @@ func (factory *StorageTypeFactory) visitObjectType( var errs []error properties := object.Properties() for i, prop := range properties { - p, err := factory.makeStorageProperty(prop, objectContext) + p, err := f.makeStorageProperty(prop, objectContext) if err != nil { errs = append(errs, err) } else { @@ -128,10 +128,10 @@ func (factory *StorageTypeFactory) visitObjectType( // makeStorageProperty applies a conversion to make a variant of the property for use when // serializing to storage -func (factory *StorageTypeFactory) makeStorageProperty( +func (f *StorageTypeFactory) makeStorageProperty( prop *astmodel.PropertyDefinition, objectContext StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { - for _, conv := range factory.propertyConversions { + for _, conv := range f.propertyConversions { p, err := conv(prop, objectContext.forProperty(prop)) if err != nil { // Something went wrong, return the error @@ -147,7 +147,7 @@ func (factory *StorageTypeFactory) makeStorageProperty( } // preserveKubernetesResourceStorageProperties preserves properties required by the KubernetesResource interface as they're always required -func (factory *StorageTypeFactory) preserveKubernetesResourceStorageProperties( +func (f *StorageTypeFactory) preserveKubernetesResourceStorageProperties( prop *astmodel.PropertyDefinition, _ StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { if astmodel.IsKubernetesResourceProperty(prop.PropertyName()) { @@ -159,7 +159,7 @@ func (factory *StorageTypeFactory) preserveKubernetesResourceStorageProperties( return nil, nil } -func (factory *StorageTypeFactory) convertPropertiesForStorage( +func (f *StorageTypeFactory) convertPropertiesForStorage( prop *astmodel.PropertyDefinition, objectContext StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { propertyType, err := factory.visitor.Visit(prop.PropertyType(), objectContext) @@ -174,7 +174,7 @@ func (factory *StorageTypeFactory) convertPropertiesForStorage( return p, nil } -func (factory *StorageTypeFactory) visitFlaggedType( +func (f *StorageTypeFactory) visitFlaggedType( tv *astmodel.TypeVisitor, flaggedType *astmodel.FlaggedType, ctx interface{}) (astmodel.Type, error) { From 4886c47515a52cbcf0ab4a7046f979e4c6522aeb Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 1 Mar 2021 16:47:09 +1300 Subject: [PATCH 05/61] Use AsEnumType() instead of hard cast --- .../pkg/codegen/storage/storage_type_factory.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 9b4761b5cd8..9229a74177a 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -69,11 +69,14 @@ func (f *StorageTypeFactory) visitTypeName(_ *astmodel.TypeVisitor, name astmode visitorContext := ctx.(StorageTypesVisitorContext) // Resolve the type name to the actual referenced type - actualDefinition, actualDefinitionFound := f.types[name] + actualType, err := f.types.FullyResolve(name) + if err != nil { + return nil, errors.Wrapf(err, "visiting type name %q", name) + } // Check for property specific handling - if visitorContext.property != nil && actualDefinitionFound { - if et, ok := actualDefinition.Type().(*astmodel.EnumType); ok { + if visitorContext.property != nil { + if et, ok := astmodel.AsEnumType(actualType); ok { // Property type refers to an enum, so we use the base type instead return et.BaseType(), nil } From 98bb10f41f77dab977778c3a7ae3471ca8cb0946 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 1 Mar 2021 17:04:24 +1300 Subject: [PATCH 06/61] Move creation of storage types into the factory --- .../codegen/pipeline_create_storage_types.go | 24 ++++------ .../codegen/storage/storage_type_factory.go | 48 +++++++++++++++---- 2 files changed, 48 insertions(+), 24 deletions(-) diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index 2d9b9df1714..6102b695e05 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -22,20 +22,12 @@ func createStorageTypes() PipelineStage { "Create storage versions of CRD types", func(ctx context.Context, types astmodel.Types) (astmodel.Types, error) { - storageFactory := storage.NewStorageTypeFactory() - visitor := storageFactory.MakeStorageTypesVisitor() - vc := storage.MakeStorageTypesVisitorContext() - var errs []error - for _, d := range types { - d := d - - if astmodel.ARMFlag.IsOn(d.Type()) { - // Skip ARM definitions, we don't need to create storage variants of those - continue - } + storageFactory := storage.NewStorageTypeFactory(types) - if _, ok := types.ResolveEnumDefinition(&d); ok { - // Skip Enum definitions as we use the base type for storage + ref, ok := name.PackageReference.AsLocalPackage() + if !ok { + // Skip definitions from non-local packages + // (should never happen) continue } @@ -54,8 +46,8 @@ func createStorageTypes() PipelineStage { return nil, err } - types.AddTypes(storageFactory.Types()) - - return types, nil + unmodified := types.Except(result) + result.AddTypes(unmodified) + return result, nil }) } diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 9229a74177a..899a1ec1c77 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -8,6 +8,7 @@ package storage import ( "fmt" "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" + "github.com/pkg/errors" kerrors "k8s.io/apimachinery/pkg/util/errors" ) @@ -18,11 +19,12 @@ type StorageTypeFactory struct { } // NewStorageTypeFactory creates a new instance of StorageTypeFactory ready for use -func NewStorageTypeFactory() *StorageTypeFactory { +func NewStorageTypeFactory(types astmodel.Types) *StorageTypeFactory { result := &StorageTypeFactory{ types: make(astmodel.Types), } + result.types.AddTypes(types) result.propertyConversions = []propertyConversion{ result.preserveKubernetesResourceStorageProperties, result.convertPropertiesForStorage, @@ -31,16 +33,46 @@ func NewStorageTypeFactory() *StorageTypeFactory { return result } -func (f *StorageTypeFactory) Add(def astmodel.TypeDefinition) { - f.types[def.Name()] = def -} +// StorageTypes returns all the storage types created by the factory, also returning any errors +// that occurred during construction +func (f *StorageTypeFactory) StorageTypes() (astmodel.Types, error) { + visitor := f.makeStorageTypesVisitor() + vc := MakeStorageTypesVisitorContext() + types := make(astmodel.Types) + var errs []error + for _, d := range types { + d := d + + if astmodel.ArmFlag.IsOn(d.Type()) { + // Skip ARM definitions, we don't need to create storage variants of those + continue + } + + if _, ok := types.ResolveEnumDefinition(&d); ok { + // Skip Enum definitions as we use the base type for storage + continue + } + + def, err := visitor.VisitDefinition(d, vc) + if err != nil { + errs = append(errs, err) + continue + } + + finalDef := def.WithDescription(f.descriptionForStorageVariant(d)) + types.Add(finalDef) + } + + if len(errs) > 0 { + err := kerrors.NewAggregate(errs) + return nil, err + } -func (f *StorageTypeFactory) Types() astmodel.Types { - return f.types + return types, nil } // makeStorageTypesVisitor returns a TypeVisitor to do the creation of dedicated storage types -func (f *StorageTypeFactory) MakeStorageTypesVisitor() astmodel.TypeVisitor { +func (f *StorageTypeFactory) makeStorageTypesVisitor() astmodel.TypeVisitor { result := astmodel.MakeTypeVisitor() result.VisitValidatedType = f.visitValidatedType @@ -189,7 +221,7 @@ func (f *StorageTypeFactory) visitFlaggedType( return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) } -func DescriptionForStorageVariant(definition astmodel.TypeDefinition) []string { +func (f *StorageTypeFactory) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { pkg := definition.Name().PackageReference.PackageName() result := []string{ From e0a896043259cad1fa051de6850ec7ef9e2ec392 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 2 Mar 2021 17:05:23 +1300 Subject: [PATCH 07/61] Use a separate factory for each group --- .../codegen/pipeline_create_storage_types.go | 18 +++++++++++++++++- .../codegen/storage/storage_type_factory.go | 10 +++++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index 6102b695e05..6c94c44211c 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -8,6 +8,7 @@ package codegen import ( "context" "github.com/Azure/k8s-infra/hack/generator/pkg/codegen/storage" + kerrors "k8s.io/apimachinery/pkg/util/errors" "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" kerrors "k8s.io/apimachinery/pkg/util/errors" @@ -22,7 +23,9 @@ func createStorageTypes() PipelineStage { "Create storage versions of CRD types", func(ctx context.Context, types astmodel.Types) (astmodel.Types, error) { - storageFactory := storage.NewStorageTypeFactory(types) + // Create a factory for each group (aka service) and divvy up the types + factories := make(map[string]*storage.StorageTypeFactory) + for name, def := range types { ref, ok := name.PackageReference.AsLocalPackage() if !ok { @@ -41,6 +44,19 @@ func createStorageTypes() PipelineStage { storageFactory.Add(finalDef) } + // Collect up all the results + result := make(astmodel.Types) + var errs []error + for _, factory := range factories { + stypes, err := factory.StorageTypes() + if err != nil { + errs = append(errs, err) + continue + } + + result.AddTypes(stypes) + } + if len(errs) > 0 { err := kerrors.NewAggregate(errs) return nil, err diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 899a1ec1c77..8b3026b7a94 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -12,6 +12,7 @@ import ( kerrors "k8s.io/apimachinery/pkg/util/errors" ) +// Each StorageTypeFactory is used to create storage types for a specific service type StorageTypeFactory struct { types astmodel.Types propertyConversions []propertyConversion @@ -19,12 +20,11 @@ type StorageTypeFactory struct { } // NewStorageTypeFactory creates a new instance of StorageTypeFactory ready for use -func NewStorageTypeFactory(types astmodel.Types) *StorageTypeFactory { +func NewStorageTypeFactory() *StorageTypeFactory { result := &StorageTypeFactory{ types: make(astmodel.Types), } - result.types.AddTypes(types) result.propertyConversions = []propertyConversion{ result.preserveKubernetesResourceStorageProperties, result.convertPropertiesForStorage, @@ -33,6 +33,10 @@ func NewStorageTypeFactory(types astmodel.Types) *StorageTypeFactory { return result } +func (f *StorageTypeFactory) Add(d astmodel.TypeDefinition) { + f.types.Add(d) +} + // StorageTypes returns all the storage types created by the factory, also returning any errors // that occurred during construction func (f *StorageTypeFactory) StorageTypes() (astmodel.Types, error) { @@ -40,7 +44,7 @@ func (f *StorageTypeFactory) StorageTypes() (astmodel.Types, error) { vc := MakeStorageTypesVisitorContext() types := make(astmodel.Types) var errs []error - for _, d := range types { + for _, d := range f.types { d := d if astmodel.ArmFlag.IsOn(d.Type()) { From 1f65c9f9dfb7ff90e18499b0c18f3548913b5556 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 2 Mar 2021 17:05:48 +1300 Subject: [PATCH 08/61] Fix bug generating storage versions of resources --- .../pkg/codegen/pipeline_ensure_type_has_arm_type.go | 8 +++++++- .../generator/pkg/codegen/storage/storage_type_factory.go | 8 +++++--- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/hack/generator/pkg/codegen/pipeline_ensure_type_has_arm_type.go b/hack/generator/pkg/codegen/pipeline_ensure_type_has_arm_type.go index dd52baeebde..0f11df45405 100644 --- a/hack/generator/pkg/codegen/pipeline_ensure_type_has_arm_type.go +++ b/hack/generator/pkg/codegen/pipeline_ensure_type_has_arm_type.go @@ -45,7 +45,13 @@ func validateAllTypesHaveARMType(definitions astmodel.Types) error { var errs []error - for _, def := range definitions { + for name, def := range definitions { + + if astmodel.IsStoragePackageReference(name.PackageReference) { + // Don't need ARM types within Storage Packages + continue + } + if resourceType, ok := definitions.ResolveResourceType(def.Type()); ok { err := findARMType(resourceType.SpecType()) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 8b3026b7a94..eb116963776 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -130,12 +130,14 @@ func (f *StorageTypeFactory) visitTypeName(_ *astmodel.TypeVisitor, name astmode } func (f *StorageTypeFactory) visitResourceType( - _ *astmodel.TypeVisitor, + tv *astmodel.TypeVisitor, resource *astmodel.ResourceType, - _ interface{}) (astmodel.Type, error) { + ctx interface{}) (astmodel.Type, error) { // storage resource types do not need defaulter interface, they have no webhooks - return resource.WithoutInterface(astmodel.DefaulterInterfaceName), nil + rsrc := resource.WithoutInterface(astmodel.DefaulterInterfaceName) + + return astmodel.IdentityVisitOfResourceType(tv, rsrc, ctx) } func (f *StorageTypeFactory) visitObjectType( From e9eec16f3c25e2abb8b2bda80525f2307e06ea22 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 23 Mar 2021 08:40:35 +1300 Subject: [PATCH 09/61] Use queue for just in time creation of storage types --- .../codegen/storage/storage_type_factory.go | 139 +++++++++++++----- 1 file changed, 106 insertions(+), 33 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index eb116963776..0f27762ffe2 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -14,15 +14,22 @@ import ( // Each StorageTypeFactory is used to create storage types for a specific service type StorageTypeFactory struct { - types astmodel.Types - propertyConversions []propertyConversion - visitor astmodel.TypeVisitor + service string // Name of the service we're handling (used mostly for logging) + types astmodel.Types // All the types for this service + propertyConversions []propertyConversion // Conversion rules to use for properties when creating storage variants + queued []astmodel.TypeDefinition // Queue of definitions needing conversion (used to trigger lazy processing) + apiTypes astmodel.Types // Modified types + storageTypes astmodel.Types // Storage variants of apiTypes + idFactory astmodel.IdentifierFactory // Factory for creating identifiers } // NewStorageTypeFactory creates a new instance of StorageTypeFactory ready for use -func NewStorageTypeFactory() *StorageTypeFactory { +func NewStorageTypeFactory(idFactory astmodel.IdentifierFactory) *StorageTypeFactory { result := &StorageTypeFactory{ - types: make(astmodel.Types), + types: make(astmodel.Types), + apiTypes: make(astmodel.Types), + storageTypes: make(astmodel.Types), + idFactory: idFactory, } result.propertyConversions = []propertyConversion{ @@ -35,67 +42,130 @@ func NewStorageTypeFactory() *StorageTypeFactory { func (f *StorageTypeFactory) Add(d astmodel.TypeDefinition) { f.types.Add(d) + + isArm := astmodel.ARMFlag.IsOn(def.Type()) + _, isEnum := astmodel.AsEnumType(def.Type()) + + if !isArm && !isEnum { + // Add to our queue of types requiring storage variants + f.queued = append(f.queued, d) + } } // StorageTypes returns all the storage types created by the factory, also returning any errors // that occurred during construction func (f *StorageTypeFactory) StorageTypes() (astmodel.Types, error) { - visitor := f.makeStorageTypesVisitor() - vc := MakeStorageTypesVisitorContext() - types := make(astmodel.Types) + err := f.processQueue() + if err != nil { + return nil, err + } + + return f.storageTypes, nil +} + +// StorageTypes returns all the API types modified by the factory, also returning any errors +// that occurred during construction +func (f *StorageTypeFactory) ModifiedTypes() (astmodel.Types, error) { + err := f.processQueue() + if err != nil { + return nil, err + } + + return f.apiTypes, nil +} + +func (f *StorageTypeFactory) processQueue() error { var errs []error - for _, d := range f.types { - d := d + visitor := f.newStorageTypesVisitor() - if astmodel.ArmFlag.IsOn(d.Type()) { - // Skip ARM definitions, we don't need to create storage variants of those + for len(f.queued) > 0 { + def := f.queued[0] + f.queued = f.queued[1:] + + if _, isObjectType := astmodel.AsObjectType(def.Type()); !isObjectType { + // Not an object type, just skip it continue } - if _, ok := types.ResolveEnumDefinition(&d); ok { - // Skip Enum definitions as we use the base type for storage + // Create our storage variant + sv, err := f.createStorageVariant(def, visitor) + if err != nil { + errs = append(errs, err) continue } - def, err := visitor.VisitDefinition(d, vc) + // Create an API variant with the necessary conversion functions + av, err := f.createApiVariant(def, sv) if err != nil { errs = append(errs, err) continue } - finalDef := def.WithDescription(f.descriptionForStorageVariant(d)) - types.Add(finalDef) + f.apiTypes.Add(av) + f.storageTypes.Add(sv) } - if len(errs) > 0 { - err := kerrors.NewAggregate(errs) - return nil, err + return kerrors.NewAggregate(errs) +} + +// createStorageVariant takes an existing object definition and creates a storage variant in a +// related package. +// def is the api definition on which to base the storage variant +// visitor is a type visitor that will do the creation +func (f *StorageTypeFactory) createStorageVariant( + def astmodel.TypeDefinition, + visitor *astmodel.TypeVisitor) (astmodel.TypeDefinition, error) { + vc := MakeStorageTypesVisitorContext() + sv, err := visitor.VisitDefinition(def, vc) + if err != nil { + return astmodel.TypeDefinition{}, errors.Wrapf(err, "creating storage variant for %q", def.Name()) } - return types, nil + desc := f.descriptionForStorageVariant(def) + return sv.WithDescription(desc), nil } -// makeStorageTypesVisitor returns a TypeVisitor to do the creation of dedicated storage types -func (f *StorageTypeFactory) makeStorageTypesVisitor() astmodel.TypeVisitor { +// createApiVariant modifies an existing object definition by adding the required conversion functions +func (f *StorageTypeFactory) createApiVariant(apiDef astmodel.TypeDefinition, storageDef astmodel.TypeDefinition) (astmodel.TypeDefinition, error) { + objectType, isObjectType := astmodel.AsObjectType(apiDef.Type()) + if !isObjectType { + return astmodel.TypeDefinition{}, errors.Errorf("Expected %q to be an object definition", apiDef.Name()) + } + + // Create conversion functions + conversionContext := astmodel.NewStorageConversionContext(f.types) + convertFrom, err := astmodel.NewStorageConversionFromFunction(apiDef, storageDef, f.idFactory, conversionContext) + if err != nil { + return astmodel.TypeDefinition{}, errors.Wrapf(err, "creating ConvertFrom() function for %q", apiDef.Name()) + } + + convertTo, err := astmodel.NewStorageConversionToFunction(apiDef, storageDef, f.idFactory, conversionContext) + if err != nil { + return astmodel.TypeDefinition{}, errors.Wrapf(err, "creating ConvertFrom() function for %q", apiDef.Name()) + } + + objectType = objectType.WithFunction(convertFrom).WithFunction(convertTo) + return apiDef.WithType(objectType), nil +} +// newStorageTypesVisitor returns a TypeVisitor to do the creation of dedicated storage types +func (f *StorageTypeFactory) newStorageTypesVisitor() *astmodel.TypeVisitor { result := astmodel.MakeTypeVisitor() result.VisitValidatedType = f.visitValidatedType result.VisitTypeName = f.visitTypeName result.VisitObjectType = f.visitObjectType result.VisitResourceType = f.visitResourceType result.VisitFlaggedType = f.visitFlaggedType - - f.visitor = result - - return result + return &result } // A property conversion accepts a property definition and optionally applies a conversion to make // the property suitable for use on a storage type. Conversions return nil if they decline to // convert, deferring the conversion to another. -type propertyConversion = func(property *astmodel.PropertyDefinition, ctx StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) +type propertyConversion = func(property *astmodel.PropertyDefinition, tv *astmodel.TypeVisitor, ctx StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) -func (f *StorageTypeFactory) visitValidatedType(this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { +func (f *StorageTypeFactory) visitValidatedType(this *astmodel.TypeVisitor, + v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { // strip all type validations from storage types, // act as if they do not exist return this.Visit(v.ElementType(), ctx) @@ -141,7 +211,7 @@ func (f *StorageTypeFactory) visitResourceType( } func (f *StorageTypeFactory) visitObjectType( - _ *astmodel.TypeVisitor, + tv *astmodel.TypeVisitor, object *astmodel.ObjectType, ctx interface{}) (astmodel.Type, error) { visitorContext := ctx.(StorageTypesVisitorContext) @@ -150,7 +220,7 @@ func (f *StorageTypeFactory) visitObjectType( var errs []error properties := object.Properties() for i, prop := range properties { - p, err := f.makeStorageProperty(prop, objectContext) + p, err := f.makeStorageProperty(prop, tv, objectContext) if err != nil { errs = append(errs, err) } else { @@ -171,9 +241,10 @@ func (f *StorageTypeFactory) visitObjectType( // serializing to storage func (f *StorageTypeFactory) makeStorageProperty( prop *astmodel.PropertyDefinition, + tv *astmodel.TypeVisitor, objectContext StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { for _, conv := range f.propertyConversions { - p, err := conv(prop, objectContext.forProperty(prop)) + p, err := conv(prop, tv, objectContext.forProperty(prop)) if err != nil { // Something went wrong, return the error return nil, err @@ -190,6 +261,7 @@ func (f *StorageTypeFactory) makeStorageProperty( // preserveKubernetesResourceStorageProperties preserves properties required by the KubernetesResource interface as they're always required func (f *StorageTypeFactory) preserveKubernetesResourceStorageProperties( prop *astmodel.PropertyDefinition, + _ *astmodel.TypeVisitor, _ StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { if astmodel.IsKubernetesResourceProperty(prop.PropertyName()) { // Keep these unchanged @@ -202,8 +274,9 @@ func (f *StorageTypeFactory) preserveKubernetesResourceStorageProperties( func (f *StorageTypeFactory) convertPropertiesForStorage( prop *astmodel.PropertyDefinition, + tv *astmodel.TypeVisitor, objectContext StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { - propertyType, err := factory.visitor.Visit(prop.PropertyType(), objectContext) + propertyType, err := tv.Visit(prop.PropertyType(), objectContext) if err != nil { return nil, err } From 14445fb1a0a9acc578c11e2f5b233e0c410b9d91 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 31 Mar 2021 14:16:50 +1300 Subject: [PATCH 10/61] Improve logging of conversion issues --- .../pkg/astmodel/local_package_reference.go | 2 +- hack/generator/pkg/astmodel/optional_type.go | 2 +- .../astmodel/storage_conversion_function.go | 28 +++++++++++++++---- hack/generator/pkg/astmodel/type_visitor.go | 16 +++++------ hack/generator/pkg/codegen/code_generator.go | 2 +- .../codegen/pipeline_create_storage_types.go | 7 ++--- .../codegen/storage/storage_type_factory.go | 5 ++-- 7 files changed, 40 insertions(+), 22 deletions(-) diff --git a/hack/generator/pkg/astmodel/local_package_reference.go b/hack/generator/pkg/astmodel/local_package_reference.go index cb90a4eedaa..8735b647b7c 100644 --- a/hack/generator/pkg/astmodel/local_package_reference.go +++ b/hack/generator/pkg/astmodel/local_package_reference.go @@ -94,7 +94,7 @@ func (pr LocalPackageReference) Equals(ref PackageReference) bool { // String returns the string representation of the package reference func (pr LocalPackageReference) String() string { - return pr.PackagePath() + return "local/" + pr.group + "/" + pr.version } // IsPreview returns true if this package reference is a preview diff --git a/hack/generator/pkg/astmodel/optional_type.go b/hack/generator/pkg/astmodel/optional_type.go index 785de60e28a..49c2db3b93e 100644 --- a/hack/generator/pkg/astmodel/optional_type.go +++ b/hack/generator/pkg/astmodel/optional_type.go @@ -104,7 +104,7 @@ func (optional *OptionalType) BaseType() Type { // String implements fmt.Stringer func (optional *OptionalType) String() string { - return fmt.Sprintf("(optional: %s)", optional.element.String()) + return fmt.Sprintf("Optional(%s)", optional.element.String()) } // Unwrap returns the type contained within the wrapper type diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index e7be50137b9..580f31a88a0 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -9,11 +9,12 @@ import ( "fmt" "go/token" "sort" + "strings" "github.com/Azure/azure-service-operator/hack/generator/pkg/astbuilder" "github.com/dave/dst" "github.com/pkg/errors" - kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/klog/v2" ) // StoragePropertyConversion represents a function that generates the correct AST to convert a single property value @@ -292,7 +293,8 @@ func (fn *StorageConversionFunction) createConversions(receiver TypeDefinition) return errors.Errorf("expected TypeDefinition %q to wrap object type, but none found", fn.otherType.Name().String()) } - var errs []error + var properties []string + first := true // Flag receiver name as used fn.knownLocals.Add(receiver.Name().Name()) @@ -314,7 +316,14 @@ func (fn *StorageConversionFunction) createConversions(receiver TypeDefinition) if err != nil { // An error was returned; this can happen even if a conversion was created as well. - errs = append(errs, err) + if first { + klog.V(2).Infof("Generating conversion function %s() for %s", fn.name, receiver.name) + first = false + } + + // err will include the details of which property, don't need to log here + klog.V(2).Infof("Error: %s", err) + properties = append(properties, string(receiverProperty.propertyName)) continue } @@ -325,7 +334,16 @@ func (fn *StorageConversionFunction) createConversions(receiver TypeDefinition) } } - return kerrors.NewAggregate(errs) + if len(properties) > 0 { + label := "properties" + if len(properties) == 1 { + label = "property" + } + klog.V(2).Infof("Errors with %d %s", len(properties), label) + return errors.Errorf("Errors with %d %s: %s", len(properties), label, strings.Join(properties, "; ")) + } + + return nil } // createPropertyConversion tries to create a property conversion between the two provided properties, using all of the @@ -344,7 +362,7 @@ func (fn *StorageConversionFunction) createPropertyConversion( if err != nil { return nil, errors.Wrapf( err, - "trying to assign %q (%s) from %q (%s)", + "trying to assign %q [%s] by converting from from %q [%s]", destinationProperty.PropertyName(), destinationProperty.PropertyType(), sourceProperty.PropertyName(), diff --git a/hack/generator/pkg/astmodel/type_visitor.go b/hack/generator/pkg/astmodel/type_visitor.go index 554d5c9980e..f693341d5f1 100644 --- a/hack/generator/pkg/astmodel/type_visitor.go +++ b/hack/generator/pkg/astmodel/type_visitor.go @@ -178,12 +178,12 @@ func IdentityVisitOfObjectType(this *TypeVisitor, it *ObjectType, ctx interface{ func IdentityVisitOfMapType(this *TypeVisitor, it *MapType, ctx interface{}) (Type, error) { visitedKey, err := this.Visit(it.key, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit map key type %T", it.key) + return nil, errors.Wrapf(err, "failed to visit map key type %q", it.key.String()) } visitedValue, err := this.Visit(it.value, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit map value type %T", it.value) + return nil, errors.Wrapf(err, "failed to visit map value type %q", it.value.String()) } if visitedKey == it.key && visitedValue == it.value { @@ -202,7 +202,7 @@ func IdentityVisitOfEnumType(_ *TypeVisitor, it *EnumType, _ interface{}) (Type, func IdentityVisitOfOptionalType(this *TypeVisitor, it *OptionalType, ctx interface{}) (Type, error) { visitedElement, err := this.Visit(it.element, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit optional element type %T", it.element) + return nil, errors.Wrapf(err, "failed to visit optional element type %q", it.element.String()) } if visitedElement == it.element { @@ -215,12 +215,12 @@ func IdentityVisitOfOptionalType(this *TypeVisitor, it *OptionalType, ctx interf func IdentityVisitOfResourceType(this *TypeVisitor, it *ResourceType, ctx interface{}) (Type, error) { visitedSpec, err := this.Visit(it.spec, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit resource spec type %T", it.spec) + return nil, errors.Wrapf(err, "failed to visit resource spec type %q", it.spec.String()) } visitedStatus, err := this.Visit(it.status, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit resource status type %T", it.status) + return nil, errors.Wrapf(err, "failed to visit resource status type %q", it.status.String()) } if visitedSpec == it.spec && visitedStatus == it.status { @@ -296,7 +296,7 @@ func typeSlicesFastEqual(t1 []Type, t2 []Type) bool { func IdentityVisitOfFlaggedType(this *TypeVisitor, ft *FlaggedType, ctx interface{}) (Type, error) { nt, err := this.Visit(ft.element, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit flagged type %T", ft.element) + return nil, errors.Wrapf(err, "failed to visit flagged type %q", ft.element.String()) } if nt == ft.element { @@ -309,7 +309,7 @@ func IdentityVisitOfFlaggedType(this *TypeVisitor, ft *FlaggedType, ctx interfac func IdentityVisitOfValidatedType(this *TypeVisitor, v *ValidatedType, ctx interface{}) (Type, error) { nt, err := this.Visit(v.element, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit validated type %T", v.element) + return nil, errors.Wrapf(err, "failed to visit validated type %q", v.element.String()) } if nt == v.element { @@ -322,7 +322,7 @@ func IdentityVisitOfValidatedType(this *TypeVisitor, v *ValidatedType, ctx inter func IdentityVisitOfErroredType(this *TypeVisitor, e *ErroredType, ctx interface{}) (Type, error) { nt, err := this.Visit(e.inner, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit errored type %T", e.inner) + return nil, errors.Wrapf(err, "failed to visit errored type %q", e.inner.String()) } if nt == e.inner { diff --git a/hack/generator/pkg/codegen/code_generator.go b/hack/generator/pkg/codegen/code_generator.go index b2fc7cf58ee..7ab9d4b6b0b 100644 --- a/hack/generator/pkg/codegen/code_generator.go +++ b/hack/generator/pkg/codegen/code_generator.go @@ -152,7 +152,7 @@ func createAllPipelineStages(idFactory astmodel.IdentifierFactory, configuration addCrossplaneEmbeddedResourceSpec(idFactory).UsedFor(CrossplaneTarget), addCrossplaneEmbeddedResourceStatus(idFactory).UsedFor(CrossplaneTarget), - createStorageTypes().UsedFor(ARMTarget), // TODO: For now only used for ARM + createStorageTypes(idFactory).UsedFor(ARMTarget), // TODO: For now only used for ARM simplifyDefinitions(), injectJsonSerializationTests(idFactory).UsedFor(ARMTarget), diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index 6c94c44211c..1a9abd6aefd 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -7,17 +7,16 @@ package codegen import ( "context" - "github.com/Azure/k8s-infra/hack/generator/pkg/codegen/storage" - kerrors "k8s.io/apimachinery/pkg/util/errors" - "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" + "github.com/Azure/azure-service-operator/hack/generator/pkg/codegen/storage" + "k8s.io/klog/v2" kerrors "k8s.io/apimachinery/pkg/util/errors" ) // createStorageTypes returns a pipeline stage that creates dedicated storage types for each resource and nested object. // Storage versions are created for *all* API versions to allow users of older versions of the operator to easily // upgrade. This is of course a bit odd for the first release, but defining the approach from day one is useful. -func createStorageTypes() PipelineStage { +func createStorageTypes(idFactory astmodel.IdentifierFactory) PipelineStage { return MakePipelineStage( "createStorage", "Create storage versions of CRD types", diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 0f27762ffe2..5bccbc648d8 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -10,6 +10,7 @@ import ( "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" "github.com/pkg/errors" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/klog/v2" ) // Each StorageTypeFactory is used to create storage types for a specific service @@ -90,7 +91,7 @@ func (f *StorageTypeFactory) processQueue() error { // Create our storage variant sv, err := f.createStorageVariant(def, visitor) if err != nil { - errs = append(errs, err) + klog.Warningf("Error creating storage variant of %s: %s", def.Name(), err) continue } @@ -141,7 +142,7 @@ func (f *StorageTypeFactory) createApiVariant(apiDef astmodel.TypeDefinition, st convertTo, err := astmodel.NewStorageConversionToFunction(apiDef, storageDef, f.idFactory, conversionContext) if err != nil { - return astmodel.TypeDefinition{}, errors.Wrapf(err, "creating ConvertFrom() function for %q", apiDef.Name()) + return astmodel.TypeDefinition{}, errors.Wrapf(err, "creating ConvertTo() function for %q", apiDef.Name()) } objectType = objectType.WithFunction(convertFrom).WithFunction(convertTo) From 1632b82c7b0c61f6f601e54596c46055864f4c5f Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Thu, 1 Apr 2021 11:28:13 +1300 Subject: [PATCH 11/61] Don't case convert endpoint names --- hack/generator/pkg/astmodel/storage_conversion_endpoint.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_endpoint.go b/hack/generator/pkg/astmodel/storage_conversion_endpoint.go index 220035593ca..33e40485f0d 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_endpoint.go +++ b/hack/generator/pkg/astmodel/storage_conversion_endpoint.go @@ -6,6 +6,7 @@ package astmodel import ( + "fmt" "strings" "github.com/gobuffalo/flect" @@ -28,7 +29,7 @@ func NewStorageConversionEndpoint( knownLocals *KnownLocalsSet) *StorageConversionEndpoint { return &StorageConversionEndpoint{ theType: theType, - name: strings.ToLower(name), + name: name, knownLocals: knownLocals, } } From 26bb92b3a8c0fd756f313b007eac9c88fa58bbb4 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Thu, 1 Apr 2021 11:32:27 +1300 Subject: [PATCH 12/61] Update StorageTypeFactory to use the name queue --- .../codegen/pipeline_create_storage_types.go | 16 +- .../codegen/storage/storage_type_factory.go | 138 ++++++++++-------- 2 files changed, 88 insertions(+), 66 deletions(-) diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index 1a9abd6aefd..888fbf7f3ca 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -33,27 +33,27 @@ func createStorageTypes(idFactory astmodel.IdentifierFactory) PipelineStage { continue } - def, err := visitor.VisitDefinition(d, vc) - if err != nil { - errs = append(errs, err) - continue + factory, ok := factories[ref.Group()] + if !ok { + klog.V(3).Infof("Creating storage factory for %s", ref.Group()) + factory = storage.NewStorageTypeFactory(idFactory) + factories[ref.Group()] = factory } - finalDef := def.WithDescription(storage.DescriptionForStorageVariant(d)) - storageFactory.Add(finalDef) + factory.Add(def) } // Collect up all the results result := make(astmodel.Types) var errs []error for _, factory := range factories { - stypes, err := factory.StorageTypes() + types, err := factory.Types() if err != nil { errs = append(errs, err) continue } - result.AddTypes(stypes) + result.AddTypes(types) } if len(errs) > 0 { diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 5bccbc648d8..0fe3795d2eb 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -15,22 +15,25 @@ import ( // Each StorageTypeFactory is used to create storage types for a specific service type StorageTypeFactory struct { - service string // Name of the service we're handling (used mostly for logging) - types astmodel.Types // All the types for this service - propertyConversions []propertyConversion // Conversion rules to use for properties when creating storage variants - queued []astmodel.TypeDefinition // Queue of definitions needing conversion (used to trigger lazy processing) + service string // Name of the service we're handling (used mostly for logging) + types astmodel.Types // All the types for this service + propertyConversions []propertyConversion // Conversion rules to use for properties when creating storage variants + pendingStorageConversion astmodel.TypeNameQueue // Queue of types that need storage variants created for them + idFactory astmodel.IdentifierFactory // Factory for creating identifiers + storageTypesVisitor *astmodel.TypeVisitor // A cached type visitor used to create storage variants +} + +/* apiTypes astmodel.Types // Modified types storageTypes astmodel.Types // Storage variants of apiTypes - idFactory astmodel.IdentifierFactory // Factory for creating identifiers -} +*/ // NewStorageTypeFactory creates a new instance of StorageTypeFactory ready for use func NewStorageTypeFactory(idFactory astmodel.IdentifierFactory) *StorageTypeFactory { result := &StorageTypeFactory{ - types: make(astmodel.Types), - apiTypes: make(astmodel.Types), - storageTypes: make(astmodel.Types), - idFactory: idFactory, + types: make(astmodel.Types), + pendingStorageConversion: astmodel.MakeTypeNameQueue(), + idFactory: idFactory, } result.propertyConversions = []propertyConversion{ @@ -38,94 +41,108 @@ func NewStorageTypeFactory(idFactory astmodel.IdentifierFactory) *StorageTypeFac result.convertPropertiesForStorage, } + result.storageTypesVisitor = result.newStorageTypesVisitor() + return result } -func (f *StorageTypeFactory) Add(d astmodel.TypeDefinition) { - f.types.Add(d) +func (f *StorageTypeFactory) Add(def astmodel.TypeDefinition) { + f.types.Add(def) isArm := astmodel.ARMFlag.IsOn(def.Type()) _, isEnum := astmodel.AsEnumType(def.Type()) if !isArm && !isEnum { // Add to our queue of types requiring storage variants - f.queued = append(f.queued, d) + f.pendingStorageConversion.Enqueue(def.Name()) } } -// StorageTypes returns all the storage types created by the factory, also returning any errors -// that occurred during construction -func (f *StorageTypeFactory) StorageTypes() (astmodel.Types, error) { - err := f.processQueue() +// Types returns types contained by the factory, including all new storage variants and modified +// api types. If any errors occur during processing, they're returned here. +func (f *StorageTypeFactory) Types() (astmodel.Types, error) { + err := f.process() if err != nil { return nil, err } - return f.storageTypes, nil + return f.types, nil } -// StorageTypes returns all the API types modified by the factory, also returning any errors -// that occurred during construction -func (f *StorageTypeFactory) ModifiedTypes() (astmodel.Types, error) { - err := f.processQueue() +func (f *StorageTypeFactory) process() error { + err := f.pendingStorageConversion.Process(f.createStorageVariant) if err != nil { - return nil, err + return err } - return f.apiTypes, nil -} + return nil + /* + var errs []error -func (f *StorageTypeFactory) processQueue() error { - var errs []error - visitor := f.newStorageTypesVisitor() - for len(f.queued) > 0 { - def := f.queued[0] - f.queued = f.queued[1:] + visitor := f.newStorageTypesVisitor() - if _, isObjectType := astmodel.AsObjectType(def.Type()); !isObjectType { - // Not an object type, just skip it - continue - } + for len(f.queued) > 0 { + def := f.queued[0] + f.queued = f.queued[1:] - // Create our storage variant - sv, err := f.createStorageVariant(def, visitor) - if err != nil { - klog.Warningf("Error creating storage variant of %s: %s", def.Name(), err) - continue - } + if _, isObjectType := astmodel.AsObjectType(def.Type()); !isObjectType { + // Not an object type, just skip it + continue + } - // Create an API variant with the necessary conversion functions - av, err := f.createApiVariant(def, sv) - if err != nil { - errs = append(errs, err) - continue + // Create our storage variant + sv, err := f.createStorageVariant(def, visitor) + if err != nil { + klog.Warningf("Error creating storage variant of %s: %s", def.Name(), err) + continue + } + + // Create an API variant with the necessary conversion functions + av, err := f.createApiVariant(def, sv) + if err != nil { + errs = append(errs, err) + continue + } + + f.apiTypes.Add(av) + f.storageTypes.Add(sv) } - f.apiTypes.Add(av) - f.storageTypes.Add(sv) - } + return kerrors.NewAggregate(errs) + */ - return kerrors.NewAggregate(errs) } // createStorageVariant takes an existing object definition and creates a storage variant in a // related package. // def is the api definition on which to base the storage variant // visitor is a type visitor that will do the creation -func (f *StorageTypeFactory) createStorageVariant( - def astmodel.TypeDefinition, - visitor *astmodel.TypeVisitor) (astmodel.TypeDefinition, error) { +func (f *StorageTypeFactory) createStorageVariant(name astmodel.TypeName) error { + + klog.V(0).Infof("Creating storage variant of %s", name) + + def, ok := f.types[name] + if !ok { + return errors.Errorf("failed to find definition for %q", name) + } + vc := MakeStorageTypesVisitorContext() - sv, err := visitor.VisitDefinition(def, vc) + storageDef, err := f.storageTypesVisitor.VisitDefinition(def, vc) if err != nil { - return astmodel.TypeDefinition{}, errors.Wrapf(err, "creating storage variant for %q", def.Name()) + return errors.Wrapf(err, "creating storage variant for %q", name) } desc := f.descriptionForStorageVariant(def) - return sv.WithDescription(desc), nil + storageDef = storageDef.WithDescription(desc) + + f.types.Add(storageDef) + //TODO: Queue for injection of conversion functions + + return nil } +/* // createApiVariant modifies an existing object definition by adding the required conversion functions func (f *StorageTypeFactory) createApiVariant(apiDef astmodel.TypeDefinition, storageDef astmodel.TypeDefinition) (astmodel.TypeDefinition, error) { objectType, isObjectType := astmodel.AsObjectType(apiDef.Type()) @@ -149,6 +166,8 @@ func (f *StorageTypeFactory) createApiVariant(apiDef astmodel.TypeDefinition, st return apiDef.WithType(objectType), nil } +*/ + // newStorageTypesVisitor returns a TypeVisitor to do the creation of dedicated storage types func (f *StorageTypeFactory) newStorageTypesVisitor() *astmodel.TypeVisitor { result := astmodel.MakeTypeVisitor() @@ -223,7 +242,7 @@ func (f *StorageTypeFactory) visitObjectType( for i, prop := range properties { p, err := f.makeStorageProperty(prop, tv, objectContext) if err != nil { - errs = append(errs, err) + errs = append(errs, errors.Wrapf(err, "property %s", prop.PropertyName())) } else { properties[i] = p } @@ -259,7 +278,8 @@ func (f *StorageTypeFactory) makeStorageProperty( return nil, fmt.Errorf("failed to find a conversion for property %v", prop.PropertyName()) } -// preserveKubernetesResourceStorageProperties preserves properties required by the KubernetesResource interface as they're always required +// preserveKubernetesResourceStorageProperties preserves properties required by the +// KubernetesResource interface as they're always required func (f *StorageTypeFactory) preserveKubernetesResourceStorageProperties( prop *astmodel.PropertyDefinition, _ *astmodel.TypeVisitor, @@ -301,6 +321,8 @@ func (f *StorageTypeFactory) visitFlaggedType( return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) } +// descriptionForStorageVariant creates a description for a storage variant, indicating which +// original type it is based upon func (f *StorageTypeFactory) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { pkg := definition.Name().PackageReference.PackageName() From 39d9bce2e0743cea0f6edbe2f375b80ddd81552d Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 20 Apr 2021 17:18:40 +1200 Subject: [PATCH 13/61] Code gardening --- .../pkg/astmodel/local_package_reference.go | 2 +- .../astmodel/storage_conversion_function.go | 4 +- .../pkg/astmodel/storage_package_reference.go | 2 +- .../codegen/storage/storage_type_factory.go | 44 +++---------------- 4 files changed, 11 insertions(+), 41 deletions(-) diff --git a/hack/generator/pkg/astmodel/local_package_reference.go b/hack/generator/pkg/astmodel/local_package_reference.go index 8735b647b7c..616b4d786b0 100644 --- a/hack/generator/pkg/astmodel/local_package_reference.go +++ b/hack/generator/pkg/astmodel/local_package_reference.go @@ -94,7 +94,7 @@ func (pr LocalPackageReference) Equals(ref PackageReference) bool { // String returns the string representation of the package reference func (pr LocalPackageReference) String() string { - return "local/" + pr.group + "/" + pr.version + return fmt.Sprintf("local:%s/%s", pr.group, pr.version) } // IsPreview returns true if this package reference is a preview diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index 580f31a88a0..d593d0a4454 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -48,9 +48,9 @@ type StorageConversionFunction struct { type StorageConversionDirection int const ( - // Indicates the conversion is from the passed other instance, populating the receiver + // ConvertFrom indicates the conversion is from the passed other instance, populating the receiver ConvertFrom = StorageConversionDirection(1) - // Indicate the conversion is to the passed other type, populating other + // ConvertTo indicates the conversion is to the passed other type, populating other ConvertTo = StorageConversionDirection(2) ) diff --git a/hack/generator/pkg/astmodel/storage_package_reference.go b/hack/generator/pkg/astmodel/storage_package_reference.go index daf4102ce4e..dd36b5489a7 100644 --- a/hack/generator/pkg/astmodel/storage_package_reference.go +++ b/hack/generator/pkg/astmodel/storage_package_reference.go @@ -33,7 +33,7 @@ func MakeStoragePackageReference(local LocalPackageReference) StoragePackageRefe // String returns the string representation of the package reference func (s StoragePackageReference) String() string { - return fmt.Sprintf("storage:%v", s.PackagePath()) + return fmt.Sprintf("storage:%s/%s", s.group, s.version) } // IsPreview returns true if this package reference is a preview diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 0fe3795d2eb..54da2f01fdf 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -41,7 +41,13 @@ func NewStorageTypeFactory(idFactory astmodel.IdentifierFactory) *StorageTypeFac result.convertPropertiesForStorage, } - result.storageTypesVisitor = result.newStorageTypesVisitor() + result.storageConverter = astmodel.TypeVisitorBuilder{ + VisitResourceType: result.convertResourceType, + VisitObjectType: result.convertObjectType, + VisitTypeName: result.redirectTypeNamesToStoragePackage, + VisitValidatedType: result.stripAllValidations, + VisitFlaggedType: result.stripAllFlags, + }.Build() return result } @@ -76,42 +82,6 @@ func (f *StorageTypeFactory) process() error { } return nil - /* - var errs []error - - - visitor := f.newStorageTypesVisitor() - - for len(f.queued) > 0 { - def := f.queued[0] - f.queued = f.queued[1:] - - if _, isObjectType := astmodel.AsObjectType(def.Type()); !isObjectType { - // Not an object type, just skip it - continue - } - - // Create our storage variant - sv, err := f.createStorageVariant(def, visitor) - if err != nil { - klog.Warningf("Error creating storage variant of %s: %s", def.Name(), err) - continue - } - - // Create an API variant with the necessary conversion functions - av, err := f.createApiVariant(def, sv) - if err != nil { - errs = append(errs, err) - continue - } - - f.apiTypes.Add(av) - f.storageTypes.Add(sv) - } - - return kerrors.NewAggregate(errs) - */ - } // createStorageVariant takes an existing object definition and creates a storage variant in a From 6f1bc352e3a227a8a3ff542a2e57c277f16c93a6 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 20 Apr 2021 17:20:52 +1200 Subject: [PATCH 14/61] Add injection of conversion functions --- .../codegen/storage/storage_type_factory.go | 93 ++++++++++++++----- 1 file changed, 72 insertions(+), 21 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 54da2f01fdf..da3f324cd15 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -15,12 +15,17 @@ import ( // Each StorageTypeFactory is used to create storage types for a specific service type StorageTypeFactory struct { - service string // Name of the service we're handling (used mostly for logging) - types astmodel.Types // All the types for this service - propertyConversions []propertyConversion // Conversion rules to use for properties when creating storage variants - pendingStorageConversion astmodel.TypeNameQueue // Queue of types that need storage variants created for them - idFactory astmodel.IdentifierFactory // Factory for creating identifiers - storageTypesVisitor *astmodel.TypeVisitor // A cached type visitor used to create storage variants + service string // Name of the service we're handling (used mostly for logging) + types astmodel.Types // All the types for this service + propertyConversions []propertyConversion // Conversion rules to use for properties when creating storage variants + pendingStorageConversion astmodel.TypeNameQueue // Queue of types that need storage variants created for them + pendingConversionInjection astmodel.TypeNameQueue // Queue of types that need conversion functions injected + idFactory astmodel.IdentifierFactory // Factory for creating identifiers + storageTypesVisitor *astmodel.TypeVisitor // A cached type visitor used to create storage variants + + // Map of conversion links for creating our conversion graph + // (Can't use PackageReferences as keys, so keyed by the full package path) + conversionMap map[string]astmodel.PackageReference } /* @@ -31,9 +36,11 @@ type StorageTypeFactory struct { // NewStorageTypeFactory creates a new instance of StorageTypeFactory ready for use func NewStorageTypeFactory(idFactory astmodel.IdentifierFactory) *StorageTypeFactory { result := &StorageTypeFactory{ - types: make(astmodel.Types), - pendingStorageConversion: astmodel.MakeTypeNameQueue(), - idFactory: idFactory, + types: make(astmodel.Types), + pendingStorageConversion: astmodel.MakeTypeNameQueue(), + pendingConversionInjection: astmodel.MakeTypeNameQueue(), + idFactory: idFactory, + conversionMap: make(map[string]astmodel.PackageReference), } result.propertyConversions = []propertyConversion{ @@ -81,6 +88,11 @@ func (f *StorageTypeFactory) process() error { return err } + err = f.pendingConversionInjection.Process(f.injectConversions) + if err != nil { + return err + } + return nil } @@ -107,33 +119,72 @@ func (f *StorageTypeFactory) createStorageVariant(name astmodel.TypeName) error storageDef = storageDef.WithDescription(desc) f.types.Add(storageDef) - //TODO: Queue for injection of conversion functions + + // Add API-Package -> Storage-Package link into the conversion map + f.conversionMap[name.PackageReference.PackagePath()] = storageDef.Name().PackageReference + + // Queue for injection of conversion functions + f.pendingConversionInjection.Enqueue(name) + + //TODO: Queue storage type for injection of conversion too return nil } -/* -// createApiVariant modifies an existing object definition by adding the required conversion functions -func (f *StorageTypeFactory) createApiVariant(apiDef astmodel.TypeDefinition, storageDef astmodel.TypeDefinition) (astmodel.TypeDefinition, error) { - objectType, isObjectType := astmodel.AsObjectType(apiDef.Type()) - if !isObjectType { - return astmodel.TypeDefinition{}, errors.Errorf("Expected %q to be an object definition", apiDef.Name()) +// injectConversions modifies the named type by injecting the required conversion methods using +// the conversionMap we've previously established +func (f *StorageTypeFactory) injectConversions(name astmodel.TypeName) error { + klog.V(3).Infof("Injecting conversion functions into %s", name) + + // Find the definition to modify + def, ok := f.types[name] + if !ok { + return errors.Errorf("failed to find definition for %q", name) + } + + // Find the definition we want to convert to/from + nextPackage, ok := f.conversionMap[name.PackageReference.PackagePath()] + nextName := astmodel.MakeTypeName(nextPackage, name.Name()) + nextDef, ok := f.types[nextName] + if !ok { + // No next type so nothing to do + // (this is expected if the type is discontinued) + return nil } // Create conversion functions conversionContext := astmodel.NewStorageConversionContext(f.types) - convertFrom, err := astmodel.NewStorageConversionFromFunction(apiDef, storageDef, f.idFactory, conversionContext) + + convertFrom, err := astmodel.NewStorageConversionFromFunction(def, nextDef, f.idFactory, conversionContext) if err != nil { - return astmodel.TypeDefinition{}, errors.Wrapf(err, "creating ConvertFrom() function for %q", apiDef.Name()) + return errors.Wrapf(err, "creating ConvertFrom() function for %q", name) } - convertTo, err := astmodel.NewStorageConversionToFunction(apiDef, storageDef, f.idFactory, conversionContext) + convertTo, err := astmodel.NewStorageConversionToFunction(def, nextDef, f.idFactory, conversionContext) if err != nil { - return astmodel.TypeDefinition{}, errors.Wrapf(err, "creating ConvertTo() function for %q", apiDef.Name()) + return errors.Wrapf(err, "creating ConvertTo() function for %q", name) } + // Update the object underlying our definition to include these functions + objectType, isObjectType := astmodel.AsObjectType(def.Type()) + if !isObjectType { + // This shouldn't happen because only objects should be added to the + // pendingConversionInjection queue + return errors.Errorf("expected %q to be an object definition", def.Name()) + } objectType = objectType.WithFunction(convertFrom).WithFunction(convertTo) - return apiDef.WithType(objectType), nil + def = def.WithType(objectType) + + // Update our map + f.types[name] = def + + return nil +} + +/* +// createApiVariant modifies an existing object definition by adding the required conversion functions +func (f *StorageTypeFactory) createApiVariant(apiDef astmodel.TypeDefinition, storageDef astmodel.TypeDefinition) (astmodel.TypeDefinition, error) { + } */ From 8d19b86743f6356c953f440362ae3c40386de7ba Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 20 Apr 2021 17:21:13 +1200 Subject: [PATCH 15/61] Improve debug diagnostics --- .../pkg/astmodel/storage_conversion_factories.go | 11 +++++++++-- .../pkg/astmodel/storage_conversion_function.go | 11 +++++++---- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_factories.go b/hack/generator/pkg/astmodel/storage_conversion_factories.go index e25627323ad..3907a808b33 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_factories.go +++ b/hack/generator/pkg/astmodel/storage_conversion_factories.go @@ -7,6 +7,7 @@ package astmodel import ( "go/token" + "strings" "github.com/Azure/azure-service-operator/hack/generator/pkg/astbuilder" "github.com/dave/dst" @@ -123,10 +124,16 @@ func createTypeConversion( } } + var debugDescriptionOfDestination strings.Builder + destinationEndpoint.Type().WriteDebugDescription(&debugDescriptionOfDestination, conversionContext.types) + + var debugDescriptionOfSource strings.Builder + sourceEndpoint.Type().WriteDebugDescription(&debugDescriptionOfSource, conversionContext.types) + err := errors.Errorf( "no conversion found to assign %q from %q", - destinationEndpoint.name, - sourceEndpoint.name) + debugDescriptionOfDestination.String(), + debugDescriptionOfSource.String()) return nil, err } diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index d593d0a4454..145b346e484 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -76,7 +76,7 @@ func NewStorageConversionFromFunction( result.name = "ConvertFrom" + version result.conversionContext = conversionContext.WithFunctionName(result.name) - err := result.createConversions(receiver) + err := result.createConversions(receiver, conversionContext.types) if err != nil { return nil, errors.Wrapf(err, "creating '%s()'", result.name) } @@ -103,7 +103,7 @@ func NewStorageConversionToFunction( result.name = "ConvertTo" + version result.conversionContext = conversionContext.WithFunctionName(result.name) - err := result.createConversions(receiver) + err := result.createConversions(receiver, conversionContext.types) if err != nil { return nil, errors.Wrapf(err, "creating '%s()'", result.name) } @@ -277,14 +277,17 @@ func (fn *StorageConversionFunction) generateAssignments( // createConversions iterates through the properties on our receiver type, matching them up with // our other type and generating conversions where possible -func (fn *StorageConversionFunction) createConversions(receiver TypeDefinition) error { +func (fn *StorageConversionFunction) createConversions(receiver TypeDefinition, types Types) error { receiverObject, ok := AsObjectType(receiver.Type()) if !ok { + var typeDescription strings.Builder + receiver.Type().WriteDebugDescription(&typeDescription, types) + return errors.Errorf( "expected TypeDefinition %q to wrap receiver object type, but found %q", receiver.name.String(), - receiver.Type()) + typeDescription.String()) } var otherObject *ObjectType From cc635d4871079daeab8c269c0d838104b4db2189 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 20 Apr 2021 17:24:46 +1200 Subject: [PATCH 16/61] Change to using TypeVisitorBuilder --- .../codegen/storage/storage_type_factory.go | 176 +++++++++--------- 1 file changed, 91 insertions(+), 85 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index da3f324cd15..8179e679fa4 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -21,18 +21,14 @@ type StorageTypeFactory struct { pendingStorageConversion astmodel.TypeNameQueue // Queue of types that need storage variants created for them pendingConversionInjection astmodel.TypeNameQueue // Queue of types that need conversion functions injected idFactory astmodel.IdentifierFactory // Factory for creating identifiers - storageTypesVisitor *astmodel.TypeVisitor // A cached type visitor used to create storage variants + storageConverter astmodel.TypeVisitor // a cached type visitor used to create storage variants + propertyConverter astmodel.TypeVisitor // a cached type visitor used to simplify property types // Map of conversion links for creating our conversion graph // (Can't use PackageReferences as keys, so keyed by the full package path) conversionMap map[string]astmodel.PackageReference } -/* - apiTypes astmodel.Types // Modified types - storageTypes astmodel.Types // Storage variants of apiTypes -*/ - // NewStorageTypeFactory creates a new instance of StorageTypeFactory ready for use func NewStorageTypeFactory(idFactory astmodel.IdentifierFactory) *StorageTypeFactory { result := &StorageTypeFactory{ @@ -45,17 +41,23 @@ func NewStorageTypeFactory(idFactory astmodel.IdentifierFactory) *StorageTypeFac result.propertyConversions = []propertyConversion{ result.preserveKubernetesResourceStorageProperties, - result.convertPropertiesForStorage, + result.defaultPropertyConversion, } result.storageConverter = astmodel.TypeVisitorBuilder{ - VisitResourceType: result.convertResourceType, VisitObjectType: result.convertObjectType, + VisitResourceType: result.convertResourceType, VisitTypeName: result.redirectTypeNamesToStoragePackage, VisitValidatedType: result.stripAllValidations, VisitFlaggedType: result.stripAllFlags, }.Build() + result.propertyConverter = astmodel.TypeVisitorBuilder{ + VisitEnumType: result.useBaseTypeForEnumerations, + VisitValidatedType: result.stripAllValidations, + VisitTypeName: result.shortCircuitNamesOfSimpleTypes, + }.Build() + return result } @@ -109,8 +111,7 @@ func (f *StorageTypeFactory) createStorageVariant(name astmodel.TypeName) error return errors.Errorf("failed to find definition for %q", name) } - vc := MakeStorageTypesVisitorContext() - storageDef, err := f.storageTypesVisitor.VisitDefinition(def, vc) + storageDef, err := f.storageConverter.VisitDefinition(def, nil) if err != nil { return errors.Wrapf(err, "creating storage variant for %q", name) } @@ -189,58 +190,22 @@ func (f *StorageTypeFactory) createApiVariant(apiDef astmodel.TypeDefinition, st */ -// newStorageTypesVisitor returns a TypeVisitor to do the creation of dedicated storage types -func (f *StorageTypeFactory) newStorageTypesVisitor() *astmodel.TypeVisitor { - result := astmodel.MakeTypeVisitor() - result.VisitValidatedType = f.visitValidatedType - result.VisitTypeName = f.visitTypeName - result.VisitObjectType = f.visitObjectType - result.VisitResourceType = f.visitResourceType - result.VisitFlaggedType = f.visitFlaggedType - return &result -} - -// A property conversion accepts a property definition and optionally applies a conversion to make -// the property suitable for use on a storage type. Conversions return nil if they decline to -// convert, deferring the conversion to another. -type propertyConversion = func(property *astmodel.PropertyDefinition, tv *astmodel.TypeVisitor, ctx StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) - -func (f *StorageTypeFactory) visitValidatedType(this *astmodel.TypeVisitor, - v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { +func (f *StorageTypeFactory) stripAllValidations( + this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { // strip all type validations from storage types, // act as if they do not exist return this.Visit(v.ElementType(), ctx) } -func (f *StorageTypeFactory) visitTypeName(_ *astmodel.TypeVisitor, name astmodel.TypeName, ctx interface{}) (astmodel.Type, error) { - visitorContext := ctx.(StorageTypesVisitorContext) - - // Resolve the type name to the actual referenced type - actualType, err := f.types.FullyResolve(name) - if err != nil { - return nil, errors.Wrapf(err, "visiting type name %q", name) - } - - // Check for property specific handling - if visitorContext.property != nil { - if et, ok := astmodel.AsEnumType(actualType); ok { - // Property type refers to an enum, so we use the base type instead - return et.BaseType(), nil - } - } - - // Map the type name into our storage namespace - localRef, ok := name.PackageReference.AsLocalPackage() - if !ok { - return name, nil +func (f *StorageTypeFactory) redirectTypeNamesToStoragePackage(_ *astmodel.TypeVisitor, name astmodel.TypeName, _ interface{}) (astmodel.Type, error) { + if result, ok := f.tryConvertToStorageNamespace(name); ok { + return result, nil } - storageRef := astmodel.MakeStoragePackageReference(localRef) - visitedName := astmodel.MakeTypeName(storageRef, name.Name()) - return visitedName, nil + return name, nil } -func (f *StorageTypeFactory) visitResourceType( +func (f *StorageTypeFactory) convertResourceType( tv *astmodel.TypeVisitor, resource *astmodel.ResourceType, ctx interface{}) (astmodel.Type, error) { @@ -251,17 +216,15 @@ func (f *StorageTypeFactory) visitResourceType( return astmodel.IdentityVisitOfResourceType(tv, rsrc, ctx) } -func (f *StorageTypeFactory) visitObjectType( - tv *astmodel.TypeVisitor, +func (f *StorageTypeFactory) convertObjectType( + _ *astmodel.TypeVisitor, object *astmodel.ObjectType, - ctx interface{}) (astmodel.Type, error) { - visitorContext := ctx.(StorageTypesVisitorContext) - objectContext := visitorContext.forObject(object) + _ interface{}) (astmodel.Type, error) { var errs []error properties := object.Properties() for i, prop := range properties { - p, err := f.makeStorageProperty(prop, tv, objectContext) + p, err := f.makeStorageProperty(prop) if err != nil { errs = append(errs, errors.Wrapf(err, "property %s", prop.PropertyName())) } else { @@ -278,14 +241,24 @@ func (f *StorageTypeFactory) visitObjectType( return astmodel.StorageFlag.ApplyTo(objectType), nil } +func (f *StorageTypeFactory) stripAllFlags( + tv *astmodel.TypeVisitor, + flaggedType *astmodel.FlaggedType, + ctx interface{}) (astmodel.Type, error) { + if flaggedType.HasFlag(astmodel.ARMFlag) { + // We don't want to do anything with ARM types + return flaggedType, nil + } + + return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) +} + // makeStorageProperty applies a conversion to make a variant of the property for use when // serializing to storage func (f *StorageTypeFactory) makeStorageProperty( - prop *astmodel.PropertyDefinition, - tv *astmodel.TypeVisitor, - objectContext StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { + prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { for _, conv := range f.propertyConversions { - p, err := conv(prop, tv, objectContext.forProperty(prop)) + p, err := conv(prop) if err != nil { // Something went wrong, return the error return nil, err @@ -299,26 +272,59 @@ func (f *StorageTypeFactory) makeStorageProperty( return nil, fmt.Errorf("failed to find a conversion for property %v", prop.PropertyName()) } +// A property conversion accepts a property definition and optionally applies a conversion to make +// the property suitable for use on a storage type. Conversions return nil if they decline to +// convert, deferring the conversion to another. +type propertyConversion = func(property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) + +func (f *StorageTypeFactory) useBaseTypeForEnumerations( + tv *astmodel.TypeVisitor, et *astmodel.EnumType, ctx interface{}) (astmodel.Type, error) { + return tv.Visit(et.BaseType(), ctx) +} + +func (f *StorageTypeFactory) shortCircuitNamesOfSimpleTypes( + tv *astmodel.TypeVisitor, tn astmodel.TypeName, ctx interface{}) (astmodel.Type, error) { + + actualType, err := f.types.FullyResolve(tn) + if err != nil { + // Can't resolve to underlying type, give up + return nil, err + } + + _, isObject := astmodel.AsObjectType(actualType) + _, isResource := astmodel.AsResourceType(actualType) + + if isObject || isResource { + // We have an object or a resource, redirect to our storage namespace if we can + if storageName, ok := f.tryConvertToStorageNamespace(tn); ok { + return storageName, nil + } + + // Otherwise just keep the name + return tn, nil + } + + // Replace the name with the underlying type + return tv.Visit(actualType, ctx) +} + // preserveKubernetesResourceStorageProperties preserves properties required by the -// KubernetesResource interface as they're always required +// KubernetesResource interface as they're always required exactly as declared func (f *StorageTypeFactory) preserveKubernetesResourceStorageProperties( - prop *astmodel.PropertyDefinition, - _ *astmodel.TypeVisitor, - _ StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { + prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { + if astmodel.IsKubernetesResourceProperty(prop.PropertyName()) { // Keep these unchanged return prop, nil } - // No opinion, defer to another conversion + // Not a kubernetes type, defer to another conversion return nil, nil } -func (f *StorageTypeFactory) convertPropertiesForStorage( - prop *astmodel.PropertyDefinition, - tv *astmodel.TypeVisitor, - objectContext StorageTypesVisitorContext) (*astmodel.PropertyDefinition, error) { - propertyType, err := tv.Visit(prop.PropertyType(), objectContext) +func (f *StorageTypeFactory) defaultPropertyConversion( + prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { + propertyType, err := f.propertyConverter.Visit(prop.PropertyType(), nil) if err != nil { return nil, err } @@ -330,18 +336,6 @@ func (f *StorageTypeFactory) convertPropertiesForStorage( return p, nil } -func (f *StorageTypeFactory) visitFlaggedType( - tv *astmodel.TypeVisitor, - flaggedType *astmodel.FlaggedType, - ctx interface{}) (astmodel.Type, error) { - if flaggedType.HasFlag(astmodel.ArmFlag) { - // We don't want to do anything with ARM types - return flaggedType, nil - } - - return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) -} - // descriptionForStorageVariant creates a description for a storage variant, indicating which // original type it is based upon func (f *StorageTypeFactory) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { @@ -354,3 +348,15 @@ func (f *StorageTypeFactory) descriptionForStorageVariant(definition astmodel.Ty return result } + +func (f *StorageTypeFactory) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { + // Map the type name into our storage namespace + localRef, ok := name.PackageReference.AsLocalPackage() + if !ok { + return astmodel.TypeName{}, false + } + + storageRef := astmodel.MakeStoragePackageReference(localRef) + visitedName := astmodel.MakeTypeName(storageRef, name.Name()) + return visitedName, true +} From ae17cc61edceb7803e63e414d46f317db76ec594 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 28 Apr 2021 16:03:03 +1200 Subject: [PATCH 17/61] Code gardening --- hack/generator/pkg/astmodel/type_flag.go | 1 + hack/generator/pkg/codegen/storage/storage_type_factory.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/hack/generator/pkg/astmodel/type_flag.go b/hack/generator/pkg/astmodel/type_flag.go index d76b0d5bf2e..72d0dcc5e03 100644 --- a/hack/generator/pkg/astmodel/type_flag.go +++ b/hack/generator/pkg/astmodel/type_flag.go @@ -29,6 +29,7 @@ func (f TypeFlag) ApplyTo(t Type) *FlaggedType { // RemoveFrom applies the tag to the provided type func (f TypeFlag) RemoveFrom(t Type) (Type, error) { + removeFlag := func(it *FlaggedType) Type { return it.WithoutFlag(f) } diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 8179e679fa4..cd4eaecb9b5 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -13,7 +13,7 @@ import ( "k8s.io/klog/v2" ) -// Each StorageTypeFactory is used to create storage types for a specific service +// StorageTypeFactory is used to create storage types for a specific service type StorageTypeFactory struct { service string // Name of the service we're handling (used mostly for logging) types astmodel.Types // All the types for this service From 59e9c9e74e9424211f8468b73d6b02f32131b0f5 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 5 May 2021 12:05:58 +1200 Subject: [PATCH 18/61] Rename member to otherDefinition --- .../astmodel/storage_conversion_function.go | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index 145b346e484..9faa5b45591 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -28,9 +28,9 @@ type StoragePropertyConversion func( type StorageConversionFunction struct { // name of this conversion function name string - // otherType is the type we are converting to (or from). This will be a type which is "closer" + // otherDefinition is the type we are converting to (or from). This will be a type which is "closer" // to the hub storage type, making this a building block of the final conversion. - otherType TypeDefinition + otherDefinition TypeDefinition // conversions is a map of all property conversions we are going to use, keyed by name of the // receiver property conversions map[string]StoragePropertyConversion @@ -65,7 +65,7 @@ func NewStorageConversionFromFunction( conversionContext *StorageConversionContext, ) (*StorageConversionFunction, error) { result := &StorageConversionFunction{ - otherType: otherType, + otherDefinition: otherType, idFactory: idFactory, conversionDirection: ConvertFrom, conversions: make(map[string]StoragePropertyConversion), @@ -92,7 +92,7 @@ func NewStorageConversionToFunction( conversionContext *StorageConversionContext, ) (*StorageConversionFunction, error) { result := &StorageConversionFunction{ - otherType: otherType, + otherDefinition: otherType, idFactory: idFactory, conversionDirection: ConvertTo, conversions: make(map[string]StoragePropertyConversion), @@ -120,14 +120,14 @@ func (fn *StorageConversionFunction) Name() string { func (fn *StorageConversionFunction) RequiredPackageReferences() *PackageReferenceSet { result := NewPackageReferenceSet( ErrorsReference, - fn.otherType.Name().PackageReference) + fn.otherDefinition.Name().PackageReference) return result } // References returns the set of types referenced by this function func (fn *StorageConversionFunction) References() TypeNameSet { - return NewTypeNameSet(fn.otherType.Name()) + return NewTypeNameSet(fn.otherDefinition.Name()) } // Equals checks to see if the supplied function is the same as this one @@ -164,10 +164,10 @@ func (fn *StorageConversionFunction) AsFunc(generationContext *CodeGenerationCon switch fn.conversionDirection { case ConvertFrom: parameterName = "source" - description = fmt.Sprintf("populates our %s from the provided source %s", receiver.Name(), fn.otherType.Name().Name()) + description = fmt.Sprintf("populates our %s from the provided source %s", receiver.Name(), fn.otherDefinition.Name().Name()) case ConvertTo: parameterName = "destination" - description = fmt.Sprintf("populates the provided destination %s from our %s", fn.otherType.Name().Name(), receiver.Name()) + description = fmt.Sprintf("populates the provided destination %s from our %s", fn.otherDefinition.Name().Name(), receiver.Name()) default: panic(fmt.Sprintf("unexpected conversion direction %q", fn.conversionDirection)) } @@ -181,14 +181,14 @@ func (fn *StorageConversionFunction) AsFunc(generationContext *CodeGenerationCon Body: fn.generateBody(receiverName, parameterName, generationContext), } - parameterPackage := generationContext.MustGetImportedPackageName(fn.otherType.Name().PackageReference) + parameterPackage := generationContext.MustGetImportedPackageName(fn.otherDefinition.Name().PackageReference) funcDetails.AddParameter( parameterName, &dst.StarExpr{ X: &dst.SelectorExpr{ X: dst.NewIdent(parameterPackage), - Sel: dst.NewIdent(fn.otherType.Name().Name()), + Sel: dst.NewIdent(fn.otherDefinition.Name().Name()), }, }) From 24a855b43a77c7ba33764311124e972f6305de4a Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 5 May 2021 12:06:51 +1200 Subject: [PATCH 19/61] Use PropertyContainer to generate conversions --- .../astmodel/storage_conversion_function.go | 44 ++++++++++++++++--- 1 file changed, 37 insertions(+), 7 deletions(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index 9faa5b45591..cb8d8787eea 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -279,31 +279,39 @@ func (fn *StorageConversionFunction) generateAssignments( // our other type and generating conversions where possible func (fn *StorageConversionFunction) createConversions(receiver TypeDefinition, types Types) error { - receiverObject, ok := AsObjectType(receiver.Type()) + receiverContainer, ok := fn.asPropertyContainer(receiver.theType) if !ok { var typeDescription strings.Builder receiver.Type().WriteDebugDescription(&typeDescription, types) return errors.Errorf( - "expected TypeDefinition %q to wrap receiver object type, but found %q", + "expected receiver TypeDefinition %q to be either resource or object type, but found %q", receiver.name.String(), typeDescription.String()) } - var otherObject *ObjectType - otherObject, ok = AsObjectType(fn.otherType.Type()) + otherContainer, ok := fn.asPropertyContainer(fn.otherDefinition.theType) if !ok { - return errors.Errorf("expected TypeDefinition %q to wrap object type, but none found", fn.otherType.Name().String()) + var typeDescription strings.Builder + fn.otherDefinition.Type().WriteDebugDescription(&typeDescription, types) + + return errors.Errorf( + "expected other TypeDefinition %q to be either resource or object type, but found %q", + fn.otherDefinition.Name().String(), + typeDescription.String()) } + receiverProperties := fn.createPropertyMap(receiverContainer) + otherProperties := fn.createPropertyMap(otherContainer) + var properties []string first := true // Flag receiver name as used fn.knownLocals.Add(receiver.Name().Name()) - for _, receiverProperty := range receiverObject.Properties() { - otherProperty, ok := otherObject.Property(receiverProperty.propertyName) + for _, receiverProperty := range receiverProperties { + otherProperty, ok := otherProperties[receiverProperty.propertyName] //TODO: Handle renames if ok { var conv StoragePropertyConversion @@ -349,6 +357,28 @@ func (fn *StorageConversionFunction) createConversions(receiver TypeDefinition, return nil } +// AsPropertyContainer converts a type into a property container +func (fn *StorageConversionFunction) asPropertyContainer(theType Type) (PropertyContainer, bool) { + switch t := theType.(type) { + case PropertyContainer: + return t, true + case MetaType: + return fn.asPropertyContainer(t.Unwrap()) + default: + return nil, false + } +} + +// createPropertyMap extracts the properties from a PropertyContainer and returns them as a map +func (fn *StorageConversionFunction) createPropertyMap(container PropertyContainer) map[PropertyName]*PropertyDefinition { + result := make(map[PropertyName]*PropertyDefinition) + for _, p := range container.Properties() { + result[p.PropertyName()] = p + } + + return result +} + // createPropertyConversion tries to create a property conversion between the two provided properties, using all of the // available conversion functions in priority order to do so. If no valid conversion could be created an error is returned. func (fn *StorageConversionFunction) createPropertyConversion( From 48dca90b59720d8597464a9cf1606048510ba34f Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 5 May 2021 12:07:05 +1200 Subject: [PATCH 20/61] Code gardening --- .../pkg/astmodel/storage_conversion_function.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index cb8d8787eea..c400b07c94a 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -17,13 +17,6 @@ import ( "k8s.io/klog/v2" ) -// StoragePropertyConversion represents a function that generates the correct AST to convert a single property value -// Different functions will be used, depending on the types of the properties to be converted. -// source is an expression for the source value that will be read. -// destination is an expression the target value that will be written. -type StoragePropertyConversion func( - source dst.Expr, destination dst.Expr, generationContext *CodeGenerationContext) []dst.Stmt - // StorageConversionFunction represents a function that performs conversions for storage versions type StorageConversionFunction struct { // name of this conversion function @@ -44,6 +37,13 @@ type StorageConversionFunction struct { conversionContext *StorageConversionContext } +// StoragePropertyConversion represents a function that generates the correct AST to convert a single property value +// Different functions will be used, depending on the types of the properties to be converted. +// source is an expression for the source value that will be read. +// destination is an expression the target value that will be written. +type StoragePropertyConversion func( + source dst.Expr, destination dst.Expr, generationContext *CodeGenerationContext) []dst.Stmt + // StorageConversionDirection specifies the direction of conversion we're implementing with this function type StorageConversionDirection int From 60b112d5c51f3a9719d5720a1fd3056c90864886 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 5 May 2021 12:09:53 +1200 Subject: [PATCH 21/61] Move visitor methods to end of file --- .../codegen/storage/storage_type_factory.go | 98 +++++-------------- 1 file changed, 22 insertions(+), 76 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index cd4eaecb9b5..6bd12bfc3d5 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -205,6 +205,10 @@ func (f *StorageTypeFactory) redirectTypeNamesToStoragePackage(_ *astmodel.TypeV return name, nil } +/* + * Functions used by the storageConverter TypeVisitor + */ + func (f *StorageTypeFactory) convertResourceType( tv *astmodel.TypeVisitor, resource *astmodel.ResourceType, @@ -241,6 +245,21 @@ func (f *StorageTypeFactory) convertObjectType( return astmodel.StorageFlag.ApplyTo(objectType), nil } +func (f *StorageTypeFactory) redirectTypeNamesToStoragePackage(_ *astmodel.TypeVisitor, name astmodel.TypeName, _ interface{}) (astmodel.Type, error) { + if result, ok := f.tryConvertToStorageNamespace(name); ok { + return result, nil + } + + return name, nil +} + +func (f *StorageTypeFactory) stripAllValidations( + this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { + // strip all type validations from storage types, + // act as if they do not exist + return this.Visit(v.ElementType(), ctx) +} + func (f *StorageTypeFactory) stripAllFlags( tv *astmodel.TypeVisitor, flaggedType *astmodel.FlaggedType, @@ -253,29 +272,9 @@ func (f *StorageTypeFactory) stripAllFlags( return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) } -// makeStorageProperty applies a conversion to make a variant of the property for use when -// serializing to storage -func (f *StorageTypeFactory) makeStorageProperty( - prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { - for _, conv := range f.propertyConversions { - p, err := conv(prop) - if err != nil { - // Something went wrong, return the error - return nil, err - } - if p != nil { - // We have the conversion we need, return it promptly - return p, nil - } - } - - return nil, fmt.Errorf("failed to find a conversion for property %v", prop.PropertyName()) -} - -// A property conversion accepts a property definition and optionally applies a conversion to make -// the property suitable for use on a storage type. Conversions return nil if they decline to -// convert, deferring the conversion to another. -type propertyConversion = func(property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) +/* + * Functions used by the propertyConverter TypeVisitor + */ func (f *StorageTypeFactory) useBaseTypeForEnumerations( tv *astmodel.TypeVisitor, et *astmodel.EnumType, ctx interface{}) (astmodel.Type, error) { @@ -307,56 +306,3 @@ func (f *StorageTypeFactory) shortCircuitNamesOfSimpleTypes( // Replace the name with the underlying type return tv.Visit(actualType, ctx) } - -// preserveKubernetesResourceStorageProperties preserves properties required by the -// KubernetesResource interface as they're always required exactly as declared -func (f *StorageTypeFactory) preserveKubernetesResourceStorageProperties( - prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { - - if astmodel.IsKubernetesResourceProperty(prop.PropertyName()) { - // Keep these unchanged - return prop, nil - } - - // Not a kubernetes type, defer to another conversion - return nil, nil -} - -func (f *StorageTypeFactory) defaultPropertyConversion( - prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { - propertyType, err := f.propertyConverter.Visit(prop.PropertyType(), nil) - if err != nil { - return nil, err - } - - p := prop.WithType(propertyType). - MakeOptional(). - WithDescription("") - - return p, nil -} - -// descriptionForStorageVariant creates a description for a storage variant, indicating which -// original type it is based upon -func (f *StorageTypeFactory) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { - pkg := definition.Name().PackageReference.PackageName() - - result := []string{ - fmt.Sprintf("Storage version of %v.%v", pkg, definition.Name().Name()), - } - result = append(result, definition.Description()...) - - return result -} - -func (f *StorageTypeFactory) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { - // Map the type name into our storage namespace - localRef, ok := name.PackageReference.AsLocalPackage() - if !ok { - return astmodel.TypeName{}, false - } - - storageRef := astmodel.MakeStoragePackageReference(localRef) - visitedName := astmodel.MakeTypeName(storageRef, name.Name()) - return visitedName, true -} From 0c02a15e2d814b0ce50bb57c638cb8d35ca104df Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 5 May 2021 12:10:52 +1200 Subject: [PATCH 22/61] Create and use visitor to inject functions --- .../codegen/storage/storage_type_factory.go | 119 ++++++++++++++---- 1 file changed, 98 insertions(+), 21 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 6bd12bfc3d5..c5eb0b67cd8 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -23,6 +23,7 @@ type StorageTypeFactory struct { idFactory astmodel.IdentifierFactory // Factory for creating identifiers storageConverter astmodel.TypeVisitor // a cached type visitor used to create storage variants propertyConverter astmodel.TypeVisitor // a cached type visitor used to simplify property types + functionInjector astmodel.TypeVisitor // a cached type visitor used to inject functions into definitions // Map of conversion links for creating our conversion graph // (Can't use PackageReferences as keys, so keyed by the full package path) @@ -58,6 +59,11 @@ func NewStorageTypeFactory(idFactory astmodel.IdentifierFactory) *StorageTypeFac VisitTypeName: result.shortCircuitNamesOfSimpleTypes, }.Build() + result.functionInjector = astmodel.TypeVisitorBuilder{ + VisitObjectType: result.injectFunctionIntoObject, + VisitResourceType: result.injectFunctionIntoResource, + }.Build() + return result } @@ -166,15 +172,16 @@ func (f *StorageTypeFactory) injectConversions(name astmodel.TypeName) error { return errors.Wrapf(err, "creating ConvertTo() function for %q", name) } - // Update the object underlying our definition to include these functions - objectType, isObjectType := astmodel.AsObjectType(def.Type()) - if !isObjectType { - // This shouldn't happen because only objects should be added to the - // pendingConversionInjection queue - return errors.Errorf("expected %q to be an object definition", def.Name()) + // Inject the functions into our type definition + def, err = f.functionInjector.VisitDefinition(def, convertFrom) + if err != nil { + return errors.Wrapf(err, "failed to inject ConvertFrom function into %q", name) + } + + def, err = f.functionInjector.VisitDefinition(def, convertTo) + if err != nil { + return errors.Wrapf(err, "failed to inject ConvertFrom function into %q", name) } - objectType = objectType.WithFunction(convertFrom).WithFunction(convertTo) - def = def.WithType(objectType) // Update our map f.types[name] = def @@ -182,27 +189,81 @@ func (f *StorageTypeFactory) injectConversions(name astmodel.TypeName) error { return nil } -/* -// createApiVariant modifies an existing object definition by adding the required conversion functions -func (f *StorageTypeFactory) createApiVariant(apiDef astmodel.TypeDefinition, storageDef astmodel.TypeDefinition) (astmodel.TypeDefinition, error) { +// makeStorageProperty applies a conversion to make a variant of the property for use when +// serializing to storage +func (f *StorageTypeFactory) makeStorageProperty( + prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { + for _, conv := range f.propertyConversions { + p, err := conv(prop) + if err != nil { + // Something went wrong, return the error + return nil, err + } + if p != nil { + // We have the conversion we need, return it promptly + return p, nil + } + } + return nil, fmt.Errorf("failed to find a conversion for property %v", prop.PropertyName()) } -*/ +// A property conversion accepts a property definition and optionally applies a conversion to make +// the property suitable for use on a storage type. Conversions return nil if they decline to +// convert, deferring the conversion to another. +type propertyConversion = func(property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) -func (f *StorageTypeFactory) stripAllValidations( - this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { - // strip all type validations from storage types, - // act as if they do not exist - return this.Visit(v.ElementType(), ctx) +// preserveKubernetesResourceStorageProperties preserves properties required by the +// KubernetesResource interface as they're always required exactly as declared +func (f *StorageTypeFactory) preserveKubernetesResourceStorageProperties( + prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { + + if astmodel.IsKubernetesResourceProperty(prop.PropertyName()) { + // Keep these unchanged + return prop, nil + } + + // Not a kubernetes type, defer to another conversion + return nil, nil } -func (f *StorageTypeFactory) redirectTypeNamesToStoragePackage(_ *astmodel.TypeVisitor, name astmodel.TypeName, _ interface{}) (astmodel.Type, error) { - if result, ok := f.tryConvertToStorageNamespace(name); ok { - return result, nil +func (f *StorageTypeFactory) defaultPropertyConversion( + prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { + propertyType, err := f.propertyConverter.Visit(prop.PropertyType(), nil) + if err != nil { + return nil, err } - return name, nil + p := prop.WithType(propertyType). + MakeOptional(). + WithDescription("") + + return p, nil +} + +// descriptionForStorageVariant creates a description for a storage variant, indicating which +// original type it is based upon +func (f *StorageTypeFactory) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { + pkg := definition.Name().PackageReference.PackageName() + + result := []string{ + fmt.Sprintf("Storage version of %v.%v", pkg, definition.Name().Name()), + } + result = append(result, definition.Description()...) + + return result +} + +func (f *StorageTypeFactory) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { + // Map the type name into our storage namespace + localRef, ok := name.PackageReference.AsLocalPackage() + if !ok { + return astmodel.TypeName{}, false + } + + storageRef := astmodel.MakeStoragePackageReference(localRef) + visitedName := astmodel.MakeTypeName(storageRef, name.Name()) + return visitedName, true } /* @@ -306,3 +367,19 @@ func (f *StorageTypeFactory) shortCircuitNamesOfSimpleTypes( // Replace the name with the underlying type return tv.Visit(actualType, ctx) } + +/* + * Functions used by the functionInjector TypeVisitor + */ + +func (f *StorageTypeFactory) injectFunctionIntoObject( + tv *astmodel.TypeVisitor, ot *astmodel.ObjectType, ctx interface{}) (astmodel.Type, error) { + fn := ctx.(astmodel.Function) + return ot.WithFunction(fn), nil +} + +func (f *StorageTypeFactory) injectFunctionIntoResource( + tv *astmodel.TypeVisitor, rt *astmodel.ResourceType, ctx interface{}) (astmodel.Type, error) { + fn := ctx.(astmodel.Function) + return rt.WithFunction(fn), nil +} From 3bb029cfcd48b32bd33d99d0c83fa40b9a961147 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 5 May 2021 12:16:00 +1200 Subject: [PATCH 23/61] Code gardening --- .../codegen/storage/storage_type_factory.go | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index c5eb0b67cd8..dd3a2e5a205 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -270,6 +270,7 @@ func (f *StorageTypeFactory) tryConvertToStorageNamespace(name astmodel.TypeName * Functions used by the storageConverter TypeVisitor */ +// convertResourceType creates a storage variation of a resource type func (f *StorageTypeFactory) convertResourceType( tv *astmodel.TypeVisitor, resource *astmodel.ResourceType, @@ -281,10 +282,9 @@ func (f *StorageTypeFactory) convertResourceType( return astmodel.IdentityVisitOfResourceType(tv, rsrc, ctx) } +// convertObjectType creates a storage variation of an object type func (f *StorageTypeFactory) convertObjectType( - _ *astmodel.TypeVisitor, - object *astmodel.ObjectType, - _ interface{}) (astmodel.Type, error) { + _ *astmodel.TypeVisitor, object *astmodel.ObjectType, _ interface{}) (astmodel.Type, error) { var errs []error properties := object.Properties() @@ -306,7 +306,9 @@ func (f *StorageTypeFactory) convertObjectType( return astmodel.StorageFlag.ApplyTo(objectType), nil } -func (f *StorageTypeFactory) redirectTypeNamesToStoragePackage(_ *astmodel.TypeVisitor, name astmodel.TypeName, _ interface{}) (astmodel.Type, error) { +// redirectTypeNamesToStoragePackage modifies TypeNames to reference the current storage package +func (f *StorageTypeFactory) redirectTypeNamesToStoragePackage( + _ *astmodel.TypeVisitor, name astmodel.TypeName, _ interface{}) (astmodel.Type, error) { if result, ok := f.tryConvertToStorageNamespace(name); ok { return result, nil } @@ -314,6 +316,7 @@ func (f *StorageTypeFactory) redirectTypeNamesToStoragePackage(_ *astmodel.TypeV return name, nil } +// stripAllValidations removes all validations func (f *StorageTypeFactory) stripAllValidations( this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { // strip all type validations from storage types, @@ -321,6 +324,7 @@ func (f *StorageTypeFactory) stripAllValidations( return this.Visit(v.ElementType(), ctx) } +// stripAllFlags removes all flags func (f *StorageTypeFactory) stripAllFlags( tv *astmodel.TypeVisitor, flaggedType *astmodel.FlaggedType, @@ -337,11 +341,15 @@ func (f *StorageTypeFactory) stripAllFlags( * Functions used by the propertyConverter TypeVisitor */ +// useBaseTypeForEnumerations replaces an enumeration with its underlying base type func (f *StorageTypeFactory) useBaseTypeForEnumerations( tv *astmodel.TypeVisitor, et *astmodel.EnumType, ctx interface{}) (astmodel.Type, error) { return tv.Visit(et.BaseType(), ctx) } +// shortCircuitNamesOfSimpleTypes redirects TypeNames that reference resources or objects into our +// storage namespace, and replaces TypeNames that point to simple types (enumerations or +// primitives) with their underlying type. func (f *StorageTypeFactory) shortCircuitNamesOfSimpleTypes( tv *astmodel.TypeVisitor, tn astmodel.TypeName, ctx interface{}) (astmodel.Type, error) { @@ -372,14 +380,18 @@ func (f *StorageTypeFactory) shortCircuitNamesOfSimpleTypes( * Functions used by the functionInjector TypeVisitor */ +// injectFunctionIntoObject takes the function provided as a context and includes it on the +// provided object type func (f *StorageTypeFactory) injectFunctionIntoObject( - tv *astmodel.TypeVisitor, ot *astmodel.ObjectType, ctx interface{}) (astmodel.Type, error) { + _ *astmodel.TypeVisitor, ot *astmodel.ObjectType, ctx interface{}) (astmodel.Type, error) { fn := ctx.(astmodel.Function) return ot.WithFunction(fn), nil } +// injectFunctionIntoResource takes the function provided as a context and includes it on the +// provided resource type func (f *StorageTypeFactory) injectFunctionIntoResource( - tv *astmodel.TypeVisitor, rt *astmodel.ResourceType, ctx interface{}) (astmodel.Type, error) { + _ *astmodel.TypeVisitor, rt *astmodel.ResourceType, ctx interface{}) (astmodel.Type, error) { fn := ctx.(astmodel.Function) return rt.WithFunction(fn), nil } From 4e7dc96b3c7ced5d31036b14b9c672fbc98aa7d5 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 5 May 2021 12:47:15 +1200 Subject: [PATCH 24/61] Make TypeFlag.IsOn() handled nested types --- hack/generator/pkg/astmodel/type_flag.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/hack/generator/pkg/astmodel/type_flag.go b/hack/generator/pkg/astmodel/type_flag.go index 72d0dcc5e03..bd1b98febfc 100644 --- a/hack/generator/pkg/astmodel/type_flag.go +++ b/hack/generator/pkg/astmodel/type_flag.go @@ -41,11 +41,15 @@ func (f TypeFlag) RemoveFrom(t Type) (Type, error) { return visitor.Visit(t, nil) } -// IsOn returns true if t is a flagged type that has this flag -func (f TypeFlag) IsOn(t Type) bool { - if ft, ok := t.(*FlaggedType); ok { - return ft.HasFlag(f) +// IsOn returns true if t is or contains a flagged type that has this flag +func (f TypeFlag) IsOn(theType Type) bool { + switch t := theType.(type) { + case *FlaggedType: + return t.HasFlag(f) + + case MetaType: + return f.IsOn(t.Unwrap()) + default: + return false } - - return false } From d68972bbfe3cdfb8fe0f79a3cb0d32b4e05f049a Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 5 May 2021 13:44:34 +1200 Subject: [PATCH 25/61] Implement AsZero for ValidatedType --- hack/generator/pkg/astmodel/validated_type.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hack/generator/pkg/astmodel/validated_type.go b/hack/generator/pkg/astmodel/validated_type.go index 1047675099e..fb9d0d2bdd2 100644 --- a/hack/generator/pkg/astmodel/validated_type.go +++ b/hack/generator/pkg/astmodel/validated_type.go @@ -176,9 +176,9 @@ func (v *ValidatedType) AsType(_ *CodeGenerationContext) dst.Expr { panic("Should never happen: validated types must either be named (handled by 'name types for CRDs' pipeline stage) or be directly under properties (handled by PropertyDefinition.AsField)") } -// AsZero panics because validated types should always be named +// AsZero returns the zero for our underlying type func (v *ValidatedType) AsZero(types Types, ctx *CodeGenerationContext) dst.Expr { - panic("Should never happen: validated types must either be named (handled by 'name types for CRDs' pipeline stage) or be directly under properties (handled by PropertyDefinition.AsField)") + return v.element.AsZero(types, ctx) } func (v *ValidatedType) References() TypeNameSet { From 8b4948b9d1ce4821c7e6b715b94f8ebf2ceb6d94 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 5 May 2021 13:45:08 +1200 Subject: [PATCH 26/61] Change implementation of type conversion skipping --- .../codegen/pipeline_create_storage_types.go | 6 +++++ .../codegen/storage/storage_type_factory.go | 27 +++++++++++++------ 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index 888fbf7f3ca..43c99479792 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -40,6 +40,12 @@ func createStorageTypes(idFactory astmodel.IdentifierFactory) PipelineStage { factories[ref.Group()] = factory } + isArm := astmodel.ARMFlag.IsOn(def.Type()) + if isArm { + // skip ARM types as they don't need storage variants + continue + } + factory.Add(def) } diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index dd3a2e5a205..7bc4d445880 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -70,13 +70,8 @@ func NewStorageTypeFactory(idFactory astmodel.IdentifierFactory) *StorageTypeFac func (f *StorageTypeFactory) Add(def astmodel.TypeDefinition) { f.types.Add(def) - isArm := astmodel.ARMFlag.IsOn(def.Type()) - _, isEnum := astmodel.AsEnumType(def.Type()) - - if !isArm && !isEnum { - // Add to our queue of types requiring storage variants - f.pendingStorageConversion.Enqueue(def.Name()) - } + // Add to our queue of types requiring storage variants + f.pendingStorageConversion.Enqueue(def.Name()) } // Types returns types contained by the factory, including all new storage variants and modified @@ -110,7 +105,23 @@ func (f *StorageTypeFactory) process() error { // visitor is a type visitor that will do the creation func (f *StorageTypeFactory) createStorageVariant(name astmodel.TypeName) error { - klog.V(0).Infof("Creating storage variant of %s", name) + // Only need to create storage variants of resources and objects + underlyingType, err := f.types.FullyResolve(name) + if err != nil { + return errors.Wrapf(err, + "expected to find underlying type for %d", + name) + } + + _, isObject := astmodel.AsObjectType(underlyingType) + _, isResource := astmodel.AsResourceType(underlyingType) + if !isObject && !isResource { + // just skip it + klog.V(4).Infof("Skipping %s as no storage variant needed", name) + return nil + } + + klog.V(3).Infof("Creating storage variant of %s", name) def, ok := f.types[name] if !ok { From 797ce203ffd878509a8d437aaf279b4cea39693a Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 5 May 2021 13:52:38 +1200 Subject: [PATCH 27/61] Code gardening --- .../codegen/storage/storage_type_factory.go | 54 ++++++++++--------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 7bc4d445880..2ff1ae90c10 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -224,6 +224,35 @@ func (f *StorageTypeFactory) makeStorageProperty( // convert, deferring the conversion to another. type propertyConversion = func(property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) +// descriptionForStorageVariant creates a description for a storage variant, indicating which +// original type it is based upon +func (f *StorageTypeFactory) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { + pkg := definition.Name().PackageReference.PackageName() + + result := []string{ + fmt.Sprintf("Storage version of %v.%v", pkg, definition.Name().Name()), + } + result = append(result, definition.Description()...) + + return result +} + +func (f *StorageTypeFactory) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { + // Map the type name into our storage namespace + localRef, ok := name.PackageReference.AsLocalPackage() + if !ok { + return astmodel.TypeName{}, false + } + + storageRef := astmodel.MakeStoragePackageReference(localRef) + visitedName := astmodel.MakeTypeName(storageRef, name.Name()) + return visitedName, true +} + +/* + * Functions used as propertyConversions + */ + // preserveKubernetesResourceStorageProperties preserves properties required by the // KubernetesResource interface as they're always required exactly as declared func (f *StorageTypeFactory) preserveKubernetesResourceStorageProperties( @@ -252,31 +281,6 @@ func (f *StorageTypeFactory) defaultPropertyConversion( return p, nil } -// descriptionForStorageVariant creates a description for a storage variant, indicating which -// original type it is based upon -func (f *StorageTypeFactory) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { - pkg := definition.Name().PackageReference.PackageName() - - result := []string{ - fmt.Sprintf("Storage version of %v.%v", pkg, definition.Name().Name()), - } - result = append(result, definition.Description()...) - - return result -} - -func (f *StorageTypeFactory) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { - // Map the type name into our storage namespace - localRef, ok := name.PackageReference.AsLocalPackage() - if !ok { - return astmodel.TypeName{}, false - } - - storageRef := astmodel.MakeStoragePackageReference(localRef) - visitedName := astmodel.MakeTypeName(storageRef, name.Name()) - return visitedName, true -} - /* * Functions used by the storageConverter TypeVisitor */ From ad39514abacb0bb6ce0d0a23c29c1339ea693d55 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 5 May 2021 20:16:00 +1200 Subject: [PATCH 28/61] Clear PackageReferenceSet --- hack/generator/pkg/astmodel/package_reference_set.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hack/generator/pkg/astmodel/package_reference_set.go b/hack/generator/pkg/astmodel/package_reference_set.go index 88a27c9fc71..933382e0ba6 100644 --- a/hack/generator/pkg/astmodel/package_reference_set.go +++ b/hack/generator/pkg/astmodel/package_reference_set.go @@ -43,6 +43,11 @@ func (set *PackageReferenceSet) Remove(ref PackageReference) { delete(set.references, ref) } +// Clear removes everything from the set +func (set *PackageReferenceSet) Clear() { + set.references = make(map[PackageReference]struct{}) +} + // Contains allows checking to see if an import is included func (set *PackageReferenceSet) Contains(ref PackageReference) bool { _, ok := set.references[ref] From 4b4cdbaeae32b0944265a8956b34a358e975a413 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 10 May 2021 17:09:23 +1200 Subject: [PATCH 29/61] Address issues identified by CI build --- .../pkg/astmodel/local_package_reference.go | 2 +- hack/generator/pkg/astmodel/optional_type.go | 2 +- .../pkg/codegen/pipeline_create_storage_types.go | 2 +- .../pkg/codegen/storage/storage_type_factory.go | 12 ++++++++++-- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/hack/generator/pkg/astmodel/local_package_reference.go b/hack/generator/pkg/astmodel/local_package_reference.go index 616b4d786b0..cb90a4eedaa 100644 --- a/hack/generator/pkg/astmodel/local_package_reference.go +++ b/hack/generator/pkg/astmodel/local_package_reference.go @@ -94,7 +94,7 @@ func (pr LocalPackageReference) Equals(ref PackageReference) bool { // String returns the string representation of the package reference func (pr LocalPackageReference) String() string { - return fmt.Sprintf("local:%s/%s", pr.group, pr.version) + return pr.PackagePath() } // IsPreview returns true if this package reference is a preview diff --git a/hack/generator/pkg/astmodel/optional_type.go b/hack/generator/pkg/astmodel/optional_type.go index 49c2db3b93e..423a374fa9b 100644 --- a/hack/generator/pkg/astmodel/optional_type.go +++ b/hack/generator/pkg/astmodel/optional_type.go @@ -104,7 +104,7 @@ func (optional *OptionalType) BaseType() Type { // String implements fmt.Stringer func (optional *OptionalType) String() string { - return fmt.Sprintf("Optional(%s)", optional.element.String()) + return fmt.Sprintf("optional(%s)", optional.element.String()) } // Unwrap returns the type contained within the wrapper type diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index 43c99479792..2695255bc02 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -36,7 +36,7 @@ func createStorageTypes(idFactory astmodel.IdentifierFactory) PipelineStage { factory, ok := factories[ref.Group()] if !ok { klog.V(3).Infof("Creating storage factory for %s", ref.Group()) - factory = storage.NewStorageTypeFactory(idFactory) + factory = storage.NewStorageTypeFactory(ref.Group(), idFactory) factories[ref.Group()] = factory } diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 2ff1ae90c10..c17355e8fd0 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -7,6 +7,7 @@ package storage import ( "fmt" + "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" "github.com/pkg/errors" kerrors "k8s.io/apimachinery/pkg/util/errors" @@ -31,8 +32,9 @@ type StorageTypeFactory struct { } // NewStorageTypeFactory creates a new instance of StorageTypeFactory ready for use -func NewStorageTypeFactory(idFactory astmodel.IdentifierFactory) *StorageTypeFactory { +func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) *StorageTypeFactory { result := &StorageTypeFactory{ + service: service, types: make(astmodel.Types), pendingStorageConversion: astmodel.MakeTypeNameQueue(), pendingConversionInjection: astmodel.MakeTypeNameQueue(), @@ -109,7 +111,7 @@ func (f *StorageTypeFactory) createStorageVariant(name astmodel.TypeName) error underlyingType, err := f.types.FullyResolve(name) if err != nil { return errors.Wrapf(err, - "expected to find underlying type for %d", + "expected to find underlying type for %q", name) } @@ -162,6 +164,12 @@ func (f *StorageTypeFactory) injectConversions(name astmodel.TypeName) error { // Find the definition we want to convert to/from nextPackage, ok := f.conversionMap[name.PackageReference.PackagePath()] + if !ok { + // No next package, so nothing to do + // (this is expected if we have the hub storage package) + return nil + } + nextName := astmodel.MakeTypeName(nextPackage, name.Name()) nextDef, ok := f.types[nextName] if !ok { From 8c9703bc03d40c78f901e464729765d79842654b Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 11 May 2021 13:30:01 +1200 Subject: [PATCH 30/61] Modify generator to reuse the err local in generated code --- .../astmodel/storage_conversion_context.go | 34 +++++++++++++++++-- .../astmodel/storage_conversion_endpoint.go | 3 -- .../astmodel/storage_conversion_factories.go | 9 +++-- .../astmodel/storage_conversion_function.go | 4 +-- 4 files changed, 41 insertions(+), 9 deletions(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_context.go b/hack/generator/pkg/astmodel/storage_conversion_context.go index b53f6c727f3..dd95f9258f5 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_context.go +++ b/hack/generator/pkg/astmodel/storage_conversion_context.go @@ -12,21 +12,32 @@ type StorageConversionContext struct { types Types // functionName is the name of the function we're currently generating functionName string + // knownLocals is a reference to our set of local variables + knownLocals *KnownLocalsSet } // NewStorageConversionContext creates a new instance of a StorageConversionContext func NewStorageConversionContext(types Types) *StorageConversionContext { return &StorageConversionContext{ - types: types, + types: types, + knownLocals: nil, } } +// WithFunctionName returns a new context with the specified function name included func (c *StorageConversionContext) WithFunctionName(name string) *StorageConversionContext { - result := NewStorageConversionContext(c.types) + result := c.clone() result.functionName = name return result } +// WithKnownLocals returns a new context with the specified known locals set referenced +func (c *StorageConversionContext) WithKnownLocals(knownLocals *KnownLocalsSet) *StorageConversionContext { + result := c.clone() + result.knownLocals = knownLocals + return result +} + // ResolveType resolves a type that might be a type name into both the name and the actual // type it references, returning true iff it was a TypeName that could be resolved func (c *StorageConversionContext) ResolveType(t Type) (TypeName, Type, bool) { @@ -42,3 +53,22 @@ func (c *StorageConversionContext) ResolveType(t Type) (TypeName, Type, bool) { return name, actualType, true } + +// TryCreateLocal returns true if the specified local was successfully created, false if it already existed +func (c *StorageConversionContext) TryCreateLocal(local string) bool { + if c.knownLocals.HasName(local) { + return false + } + + c.knownLocals.Add(local) + return true +} + +// clone returns a new independent copy of this context +func (c *StorageConversionContext) clone() *StorageConversionContext { + return &StorageConversionContext{ + types: c.types, + functionName: c.functionName, + knownLocals: c.knownLocals, + } +} diff --git a/hack/generator/pkg/astmodel/storage_conversion_endpoint.go b/hack/generator/pkg/astmodel/storage_conversion_endpoint.go index 33e40485f0d..d26d211210d 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_endpoint.go +++ b/hack/generator/pkg/astmodel/storage_conversion_endpoint.go @@ -6,9 +6,6 @@ package astmodel import ( - "fmt" - "strings" - "github.com/gobuffalo/flect" ) diff --git a/hack/generator/pkg/astmodel/storage_conversion_factories.go b/hack/generator/pkg/astmodel/storage_conversion_factories.go index 3907a808b33..c9b705f27dc 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_factories.go +++ b/hack/generator/pkg/astmodel/storage_conversion_factories.go @@ -811,6 +811,11 @@ func assignObjectTypeFromObjectType( return func(reader dst.Expr, writer func(dst.Expr) []dst.Stmt, generationContext *CodeGenerationContext) []dst.Stmt { + tok := token.ASSIGN + if conversionContext.TryCreateLocal("err") { + tok = token.DEFINE + } + localId := dst.NewIdent(copyVar) errLocal := dst.NewIdent("err") @@ -829,12 +834,12 @@ func assignObjectTypeFromObjectType( if destinationName.PackageReference.Equals(generationContext.CurrentPackage()) { conversion = astbuilder.SimpleAssignment( errLocal, - token.DEFINE, + tok, astbuilder.CallExpr(localId, conversionContext.functionName, actualReader)) } else { conversion = astbuilder.SimpleAssignment( errLocal, - token.DEFINE, + tok, astbuilder.CallExpr(reader, conversionContext.functionName, localId)) } diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index c400b07c94a..034e787b1cb 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -74,7 +74,7 @@ func NewStorageConversionFromFunction( version := idFactory.CreateIdentifier(otherType.Name().PackageReference.PackageName(), Exported) result.name = "ConvertFrom" + version - result.conversionContext = conversionContext.WithFunctionName(result.name) + result.conversionContext = conversionContext.WithFunctionName(result.name).WithKnownLocals(result.knownLocals) err := result.createConversions(receiver, conversionContext.types) if err != nil { @@ -101,7 +101,7 @@ func NewStorageConversionToFunction( version := idFactory.CreateIdentifier(otherType.Name().PackageReference.PackageName(), Exported) result.name = "ConvertTo" + version - result.conversionContext = conversionContext.WithFunctionName(result.name) + result.conversionContext = conversionContext.WithFunctionName(result.name).WithKnownLocals(result.knownLocals) err := result.createConversions(receiver, conversionContext.types) if err != nil { From 78412a3693feac8bb1baf3e2b98bef8ad5cf5b14 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 11 May 2021 13:36:10 +1200 Subject: [PATCH 31/61] Remove variable shadowing --- hack/generator/pkg/codegen/pipeline_create_storage_types.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index 2695255bc02..126142a0679 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -53,13 +53,13 @@ func createStorageTypes(idFactory astmodel.IdentifierFactory) PipelineStage { result := make(astmodel.Types) var errs []error for _, factory := range factories { - types, err := factory.Types() + t, err := factory.Types() if err != nil { errs = append(errs, err) continue } - result.AddTypes(types) + result.AddTypes(t) } if len(errs) > 0 { From d13be360af192bfa453d27085a04213b64116f06 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 11 May 2021 13:50:46 +1200 Subject: [PATCH 32/61] Resources need to use function references too --- hack/generator/pkg/astmodel/resource_type.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hack/generator/pkg/astmodel/resource_type.go b/hack/generator/pkg/astmodel/resource_type.go index c7fe081a153..86d73ca7f0b 100644 --- a/hack/generator/pkg/astmodel/resource_type.go +++ b/hack/generator/pkg/astmodel/resource_type.go @@ -362,6 +362,10 @@ func (resource *ResourceType) RequiredPackageReferences() *PackageReferenceSet { references.Merge(resource.status.RequiredPackageReferences()) } + for _, fn := range resource.functions { + references.Merge(fn.RequiredPackageReferences()) + } + // Interface imports references.Merge(resource.InterfaceImplementer.RequiredPackageReferences()) From 5d55e57422676a3947d481f3a95db0e1c0efcd02 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 11 May 2021 14:04:10 +1200 Subject: [PATCH 33/61] Ensure "errors" is imported --- hack/generator/pkg/astbuilder/builder.go | 4 +-- hack/generator/pkg/astmodel/std_references.go | 28 +++++++++---------- .../astmodel/storage_conversion_factories.go | 3 ++ ...weenOptionalObjectAndOptionalObject.golden | 5 +++- ...weenRequiredObjectAndOptionalObject.golden | 5 +++- ...weenRequiredObjectAndRequiredObject.golden | 5 +++- 6 files changed, 31 insertions(+), 19 deletions(-) diff --git a/hack/generator/pkg/astbuilder/builder.go b/hack/generator/pkg/astbuilder/builder.go index caf5ae3dd1a..bad167d77c8 100644 --- a/hack/generator/pkg/astbuilder/builder.go +++ b/hack/generator/pkg/astbuilder/builder.go @@ -291,9 +291,9 @@ func ReturnNoError() dst.Stmt { // // errors.Wrap(err, ) // -func WrappedErrorf(template string, args ...interface{}) dst.Expr { +func WrappedErrorf(errorsPackage string, template string, args ...interface{}) dst.Expr { return CallQualifiedFunc( - "errors", + errorsPackage, "Wrap", dst.NewIdent("err"), StringLiteralf(template, args...)) diff --git a/hack/generator/pkg/astmodel/std_references.go b/hack/generator/pkg/astmodel/std_references.go index 85de1b69d91..a1102f84c11 100644 --- a/hack/generator/pkg/astmodel/std_references.go +++ b/hack/generator/pkg/astmodel/std_references.go @@ -7,31 +7,31 @@ package astmodel var ( // References to standard Go Libraries - ErrorsReference PackageReference = MakeExternalPackageReference("errors") - FmtReference PackageReference = MakeExternalPackageReference("fmt") - JsonReference PackageReference = MakeExternalPackageReference("encoding/json") - ReflectReference PackageReference = MakeExternalPackageReference("reflect") - TestingReference PackageReference = MakeExternalPackageReference("testing") + FmtReference = MakeExternalPackageReference("fmt") + JsonReference = MakeExternalPackageReference("encoding/json") + ReflectReference = MakeExternalPackageReference("reflect") + TestingReference = MakeExternalPackageReference("testing") // References to our Libraries GenRuntimeReference PackageReference = MakeExternalPackageReference(genRuntimePathPrefix) // References to other libraries + ErrorsReference = MakeExternalPackageReference("github.com/pkg/errors") ApiExtensionsReference = MakeExternalPackageReference("k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1") ApiExtensionsJsonReference = MakeExternalPackageReference("k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/JSON") // References to libraries used for testing - CmpReference PackageReference = MakeExternalPackageReference("github.com/google/go-cmp/cmp") - CmpOptsReference PackageReference = MakeExternalPackageReference("github.com/google/go-cmp/cmp/cmpopts") - DiffReference PackageReference = MakeExternalPackageReference("github.com/kylelemons/godebug/diff") - GopterReference PackageReference = MakeExternalPackageReference("github.com/leanovate/gopter") - GopterGenReference PackageReference = MakeExternalPackageReference("github.com/leanovate/gopter/gen") - GopterPropReference PackageReference = MakeExternalPackageReference("github.com/leanovate/gopter/prop") - GomegaReference PackageReference = MakeExternalPackageReference("github.com/onsi/gomega") - PrettyReference PackageReference = MakeExternalPackageReference("github.com/kr/pretty") + CmpReference = MakeExternalPackageReference("github.com/google/go-cmp/cmp") + CmpOptsReference = MakeExternalPackageReference("github.com/google/go-cmp/cmp/cmpopts") + DiffReference = MakeExternalPackageReference("github.com/kylelemons/godebug/diff") + GopterReference = MakeExternalPackageReference("github.com/leanovate/gopter") + GopterGenReference = MakeExternalPackageReference("github.com/leanovate/gopter/gen") + GopterPropReference = MakeExternalPackageReference("github.com/leanovate/gopter/prop") + GomegaReference = MakeExternalPackageReference("github.com/onsi/gomega") + PrettyReference = MakeExternalPackageReference("github.com/kr/pretty") // Imports with specified names - GomegaImport PackageImport = NewPackageImport(GomegaReference).WithName(".") + GomegaImport = NewPackageImport(GomegaReference).WithName(".") // Type names ResourceReferenceTypeName = MakeTypeName(GenRuntimeReference, "ResourceReference") diff --git a/hack/generator/pkg/astmodel/storage_conversion_factories.go b/hack/generator/pkg/astmodel/storage_conversion_factories.go index c9b705f27dc..b1056db9f02 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_factories.go +++ b/hack/generator/pkg/astmodel/storage_conversion_factories.go @@ -819,6 +819,8 @@ func assignObjectTypeFromObjectType( localId := dst.NewIdent(copyVar) errLocal := dst.NewIdent("err") + errorsPackageName := generationContext.MustGetImportedPackageName(ErrorsReference) + declaration := astbuilder.LocalVariableDeclaration(copyVar, createTypeDeclaration(destinationName, generationContext), "") // If our reader is a dereference, we strip that off (because we need a pointer), else we @@ -846,6 +848,7 @@ func assignObjectTypeFromObjectType( checkForError := astbuilder.ReturnIfNotNil( errLocal, astbuilder.WrappedErrorf( + errorsPackageName, "populating %s from %s, calling %s()", destinationEndpoint.name, sourceEndpoint.name, conversionContext.functionName)) diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden index e8591f75d64..69a1a751e3d 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden @@ -3,7 +3,10 @@ // Licensed under the MIT license. package vCurrent -import "github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext" +import ( + "github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext" + "github.com/pkg/errors" +) type Person struct { role *Release `json:"role"` diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden index ec385c54422..9bd9660cc0a 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden @@ -3,7 +3,10 @@ // Licensed under the MIT license. package vCurrent -import "github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext" +import ( + "github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext" + "github.com/pkg/errors" +) type Person struct { role Release `json:"role"` diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndRequiredObject.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndRequiredObject.golden index 30b0e2f3a71..24bf391d73b 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndRequiredObject.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndRequiredObject.golden @@ -3,7 +3,10 @@ // Licensed under the MIT license. package vCurrent -import "github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext" +import ( + "github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext" + "github.com/pkg/errors" +) type Person struct { role Release `json:"role"` From 0301500484d053d4e12174c5ed486b446aa90af6 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 11 May 2021 14:31:58 +1200 Subject: [PATCH 34/61] Use nested conversion contexts where needed --- .../astmodel/storage_conversion_context.go | 8 +++++++ .../astmodel/storage_conversion_factories.go | 23 ++++++++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_context.go b/hack/generator/pkg/astmodel/storage_conversion_context.go index dd95f9258f5..a6570a713b0 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_context.go +++ b/hack/generator/pkg/astmodel/storage_conversion_context.go @@ -38,6 +38,14 @@ func (c *StorageConversionContext) WithKnownLocals(knownLocals *KnownLocalsSet) return result } +// NestedContext returns a nested context with independent knownLocals so that independent blocks +// can declare and reuse locals independently +func (c *StorageConversionContext) NestedContext() *StorageConversionContext { + result := c.clone() + result.knownLocals = c.knownLocals.Clone() + return result +} + // ResolveType resolves a type that might be a type name into both the name and the actual // type it references, returning true iff it was a TypeName that could be resolved func (c *StorageConversionContext) ResolveType(t Type) (TypeName, Type, bool) { diff --git a/hack/generator/pkg/astmodel/storage_conversion_factories.go b/hack/generator/pkg/astmodel/storage_conversion_factories.go index b1056db9f02..3b9a7ed1032 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_factories.go +++ b/hack/generator/pkg/astmodel/storage_conversion_factories.go @@ -207,8 +207,13 @@ func assignFromOptionalType( } // Require a conversion between the unwrapped type and our source + // We supply a nested context as we will wrap the code generated by the conversion (if any) + // within a scope, making it independent of other code generated unwrappedEndpoint := sourceEndpoint.WithType(sourceOptional.element) - conversion, _ := createTypeConversion(unwrappedEndpoint, destinationEndpoint, conversionContext) + conversion, _ := createTypeConversion( + unwrappedEndpoint, + destinationEndpoint, + conversionContext.NestedContext()) if conversion == nil { return nil } @@ -520,9 +525,14 @@ func assignArrayFromArray( } // Require a conversion between the array types + // We supply a nested context as we will wrap the code generated by the conversion (if any) + // within a loop, making it independent of other code generated unwrappedSourceEndpoint := sourceEndpoint.WithType(sourceArray.element) unwrappedDestinationEndpoint := destinationEndpoint.WithType(destinationArray.element) - conversion, _ := createTypeConversion(unwrappedSourceEndpoint, unwrappedDestinationEndpoint, conversionContext) + conversion, _ := createTypeConversion( + unwrappedSourceEndpoint, + unwrappedDestinationEndpoint, + conversionContext.NestedContext()) if conversion == nil { return nil } @@ -599,9 +609,14 @@ func assignMapFromMap( } // Require a conversion between the map items + // We supply a nested context as we will wrap the code generated by the conversion (if any) + // within a loop, making it independent of other code generated unwrappedSourceEndpoint := sourceEndpoint.WithType(sourceMap.value) unwrappedDestinationEndpoint := destinationEndpoint.WithType(destinationMap.value) - conversion, _ := createTypeConversion(unwrappedSourceEndpoint, unwrappedDestinationEndpoint, conversionContext) + conversion, _ := createTypeConversion( + unwrappedSourceEndpoint, + unwrappedDestinationEndpoint, + conversionContext.NestedContext()) if conversion == nil { return nil } @@ -811,6 +826,8 @@ func assignObjectTypeFromObjectType( return func(reader dst.Expr, writer func(dst.Expr) []dst.Stmt, generationContext *CodeGenerationContext) []dst.Stmt { + // We have to do this at render time in order to ensure the first conversion generated + // declares 'err`, not a later one tok := token.ASSIGN if conversionContext.TryCreateLocal("err") { tok = token.DEFINE From 42fce855820b2e25016f2c2c9ec7eaf6c8703b84 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 11 May 2021 15:22:41 +1200 Subject: [PATCH 35/61] Use pointer receiver for converion types --- hack/generator/pkg/astmodel/storage_conversion_function.go | 2 +- .../testdata/ConvertBetweenAliasAndAliasType.golden | 4 ++-- .../astmodel/testdata/ConvertBetweenAliasAndBaseType.golden | 4 ++-- .../testdata/ConvertBetweenAliasAndOptionalAliasType.golden | 4 ++-- .../astmodel/testdata/ConvertBetweenEnumAndBaseType.golden | 4 ++-- .../testdata/ConvertBetweenEnumAndOptionalBaseType.golden | 4 ++-- .../testdata/ConvertBetweenOptionalAliasAndBaseType.golden | 4 ++-- .../ConvertBetweenOptionalAliasAndOptionalAliasType.golden | 4 ++-- .../ConvertBetweenOptionalAliasAndOptionalBaseType.golden | 4 ++-- .../testdata/ConvertBetweenOptionalEnumAndBaseType.golden | 4 ++-- .../ConvertBetweenOptionalEnumAndOptionalBaseType.golden | 4 ++-- .../ConvertBetweenOptionalEnumAndOptionalEnum.golden | 4 ++-- .../ConvertBetweenOptionalObjectAndOptionalObject.golden | 6 +++--- .../ConvertBetweenRequiredEnumAndOptionalEnum.golden | 4 ++-- .../ConvertBetweenRequiredEnumAndRequiredEnum.golden | 4 ++-- .../ConvertBetweenRequiredObjectAndOptionalObject.golden | 6 +++--- .../ConvertBetweenRequiredObjectAndRequiredObject.golden | 6 +++--- hack/generator/pkg/astmodel/testdata/NastyTest.golden | 4 ++-- .../testdata/SetArrayOfOptionalFromArrayOfRequired.golden | 4 ++-- .../testdata/SetArrayOfRequiredFromArrayOfOptional.golden | 4 ++-- .../testdata/SetArrayOfRequiredFromArrayOfRequired.golden | 4 ++-- hack/generator/pkg/astmodel/testdata/SetIntFromInt.golden | 4 ++-- .../pkg/astmodel/testdata/SetIntFromOptionalInt.golden | 4 ++-- .../testdata/SetMapOfOptionalFromMapOfRequired.golden | 4 ++-- .../testdata/SetMapOfRequiredFromMapOfOptional.golden | 4 ++-- .../testdata/SetMapOfRequiredFromMapOfRequired.golden | 4 ++-- .../testdata/SetOptionalStringFromOptionalString.golden | 4 ++-- .../astmodel/testdata/SetOptionalStringFromString.golden | 4 ++-- .../astmodel/testdata/SetStringFromOptionalString.golden | 4 ++-- .../pkg/astmodel/testdata/SetStringFromString.golden | 4 ++-- 30 files changed, 62 insertions(+), 62 deletions(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index 034e787b1cb..73873f0872d 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -176,7 +176,7 @@ func (fn *StorageConversionFunction) AsFunc(generationContext *CodeGenerationCon funcDetails := &astbuilder.FuncDetails{ ReceiverIdent: receiverName, - ReceiverType: receiver.AsType(generationContext), + ReceiverType: NewOptionalType(receiver).AsType(generationContext), Name: fn.Name(), Body: fn.generateBody(receiverName, parameterName, generationContext), } diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndAliasType.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndAliasType.golden index 2b8ca78e9b3..28318dde7e3 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndAliasType.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndAliasType.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // age person.age = Age(source.age) @@ -20,7 +20,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // age destination.age = vNext.Age(person.age) diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndBaseType.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndBaseType.golden index 5507c3c28ef..e52686a36a9 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndBaseType.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndBaseType.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // age person.age = Age(source.age) @@ -20,7 +20,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // age destination.age = int(person.age) diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndOptionalAliasType.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndOptionalAliasType.golden index d4507a7f398..b9286182b77 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndOptionalAliasType.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenAliasAndOptionalAliasType.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // age if source.age != nil { @@ -24,7 +24,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // age age := vNext.Age(person.age) diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenEnumAndBaseType.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenEnumAndBaseType.golden index a4b7c8fa7da..151d85d3d40 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenEnumAndBaseType.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenEnumAndBaseType.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // sku person.sku = string(source.sku) @@ -20,7 +20,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // sku destination.sku = vNext.Bucket(person.sku) diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenEnumAndOptionalBaseType.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenEnumAndOptionalBaseType.golden index 87f22d64883..979dbdb1c79 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenEnumAndOptionalBaseType.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenEnumAndOptionalBaseType.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // sku sku := string(source.sku) @@ -21,7 +21,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // sku if person.sku != nil { diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndBaseType.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndBaseType.golden index b6388df860c..ed4ebb45e34 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndBaseType.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndBaseType.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // age age := Age(source.age) @@ -21,7 +21,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // age if person.age != nil { diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndOptionalAliasType.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndOptionalAliasType.golden index 126514118e4..c28d0a4aa96 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndOptionalAliasType.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndOptionalAliasType.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // age if source.age != nil { @@ -25,7 +25,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // age if person.age != nil { diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndOptionalBaseType.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndOptionalBaseType.golden index 45671365c74..6e79ac84a49 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndOptionalBaseType.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalAliasAndOptionalBaseType.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // age if source.age != nil { @@ -25,7 +25,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // age if person.age != nil { diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndBaseType.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndBaseType.golden index c4c12af18e7..7fd311fc495 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndBaseType.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndBaseType.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // sku if source.sku != nil { @@ -24,7 +24,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // sku sku := vNext.Bucket(person.sku) diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndOptionalBaseType.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndOptionalBaseType.golden index 7e6ab95cfba..2d1755dc949 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndOptionalBaseType.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndOptionalBaseType.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // sku if source.sku != nil { @@ -25,7 +25,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // sku if person.sku != nil { diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndOptionalEnum.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndOptionalEnum.golden index 1cae259eaf9..47c9b8367bf 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndOptionalEnum.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalEnumAndOptionalEnum.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // release if source.release != nil { @@ -25,7 +25,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // release if person.release != nil { diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden index 69a1a751e3d..e758ccd5957 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden @@ -13,7 +13,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // role if source.role != nil { @@ -32,12 +32,12 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // role if person.role != nil { var role vNext.Release - err := (*person.role).ConvertToVNext(role) + err := (*person.role).ConvertToVNext(&role) if err != nil { return errors.Wrap(err, "populating role from role, calling ConvertToVNext()") } diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredEnumAndOptionalEnum.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredEnumAndOptionalEnum.golden index 766fa5c81a3..155f25746c1 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredEnumAndOptionalEnum.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredEnumAndOptionalEnum.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // release if source.release != nil { @@ -25,7 +25,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // release release := Bucket(person.release) diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredEnumAndRequiredEnum.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredEnumAndRequiredEnum.golden index 0feb7688144..4ed51d4b221 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredEnumAndRequiredEnum.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredEnumAndRequiredEnum.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // release release := Bucket(source.release) @@ -21,7 +21,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // release release := Bucket(person.release) diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden index 9bd9660cc0a..8b0f51ca39b 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden @@ -13,7 +13,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // role if source.role != nil { @@ -32,11 +32,11 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // role var role vNext.Release - err := person.role.ConvertToVNext(role) + err := person.role.ConvertToVNext(&role) if err != nil { return errors.Wrap(err, "populating role from role, calling ConvertToVNext()") } diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndRequiredObject.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndRequiredObject.golden index 24bf391d73b..13652e676ab 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndRequiredObject.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndRequiredObject.golden @@ -13,7 +13,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // role var role Release @@ -28,11 +28,11 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // role var role vNext.Release - err := person.role.ConvertToVNext(role) + err := person.role.ConvertToVNext(&role) if err != nil { return errors.Wrap(err, "populating role from role, calling ConvertToVNext()") } diff --git a/hack/generator/pkg/astmodel/testdata/NastyTest.golden b/hack/generator/pkg/astmodel/testdata/NastyTest.golden index 5df4413eb1b..5680232274f 100644 --- a/hack/generator/pkg/astmodel/testdata/NastyTest.golden +++ b/hack/generator/pkg/astmodel/testdata/NastyTest.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // nasty nastyMap := make(map[string][]map[string]bool) @@ -38,7 +38,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // nasty nastyMap := make(map[string][]map[string]bool) diff --git a/hack/generator/pkg/astmodel/testdata/SetArrayOfOptionalFromArrayOfRequired.golden b/hack/generator/pkg/astmodel/testdata/SetArrayOfOptionalFromArrayOfRequired.golden index ebd8214beca..3be053b478f 100644 --- a/hack/generator/pkg/astmodel/testdata/SetArrayOfOptionalFromArrayOfRequired.golden +++ b/hack/generator/pkg/astmodel/testdata/SetArrayOfOptionalFromArrayOfRequired.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // scores scoreList := make([]*int, len(source.scores)) @@ -26,7 +26,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // scores scoreList := make([]int, len(person.scores)) diff --git a/hack/generator/pkg/astmodel/testdata/SetArrayOfRequiredFromArrayOfOptional.golden b/hack/generator/pkg/astmodel/testdata/SetArrayOfRequiredFromArrayOfOptional.golden index ee4dbac985a..9e1b25fd151 100644 --- a/hack/generator/pkg/astmodel/testdata/SetArrayOfRequiredFromArrayOfOptional.golden +++ b/hack/generator/pkg/astmodel/testdata/SetArrayOfRequiredFromArrayOfOptional.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // scores scoreList := make([]int, len(source.scores)) @@ -30,7 +30,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // scores scoreList := make([]*int, len(person.scores)) diff --git a/hack/generator/pkg/astmodel/testdata/SetArrayOfRequiredFromArrayOfRequired.golden b/hack/generator/pkg/astmodel/testdata/SetArrayOfRequiredFromArrayOfRequired.golden index 3900b5bbdfc..381c1f08ad0 100644 --- a/hack/generator/pkg/astmodel/testdata/SetArrayOfRequiredFromArrayOfRequired.golden +++ b/hack/generator/pkg/astmodel/testdata/SetArrayOfRequiredFromArrayOfRequired.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // scores scoreList := make([]int, len(source.scores)) @@ -26,7 +26,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // scores scoreList := make([]int, len(person.scores)) diff --git a/hack/generator/pkg/astmodel/testdata/SetIntFromInt.golden b/hack/generator/pkg/astmodel/testdata/SetIntFromInt.golden index 7ae92611bae..31899886562 100644 --- a/hack/generator/pkg/astmodel/testdata/SetIntFromInt.golden +++ b/hack/generator/pkg/astmodel/testdata/SetIntFromInt.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // age person.age = source.age @@ -20,7 +20,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // age destination.age = person.age diff --git a/hack/generator/pkg/astmodel/testdata/SetIntFromOptionalInt.golden b/hack/generator/pkg/astmodel/testdata/SetIntFromOptionalInt.golden index a058e477d08..211d8c670a0 100644 --- a/hack/generator/pkg/astmodel/testdata/SetIntFromOptionalInt.golden +++ b/hack/generator/pkg/astmodel/testdata/SetIntFromOptionalInt.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // age if source.age != nil { @@ -24,7 +24,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // age age := person.age diff --git a/hack/generator/pkg/astmodel/testdata/SetMapOfOptionalFromMapOfRequired.golden b/hack/generator/pkg/astmodel/testdata/SetMapOfOptionalFromMapOfRequired.golden index 2c05e78a4be..94d098ce7e7 100644 --- a/hack/generator/pkg/astmodel/testdata/SetMapOfOptionalFromMapOfRequired.golden +++ b/hack/generator/pkg/astmodel/testdata/SetMapOfOptionalFromMapOfRequired.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // ratings ratingMap := make(map[string]*int) @@ -26,7 +26,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // ratings ratingMap := make(map[string]int) diff --git a/hack/generator/pkg/astmodel/testdata/SetMapOfRequiredFromMapOfOptional.golden b/hack/generator/pkg/astmodel/testdata/SetMapOfRequiredFromMapOfOptional.golden index 22cdadf802e..68b71fe3fd2 100644 --- a/hack/generator/pkg/astmodel/testdata/SetMapOfRequiredFromMapOfOptional.golden +++ b/hack/generator/pkg/astmodel/testdata/SetMapOfRequiredFromMapOfOptional.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // ratings ratingMap := make(map[string]int) @@ -30,7 +30,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // ratings ratingMap := make(map[string]*int) diff --git a/hack/generator/pkg/astmodel/testdata/SetMapOfRequiredFromMapOfRequired.golden b/hack/generator/pkg/astmodel/testdata/SetMapOfRequiredFromMapOfRequired.golden index 076df0f79e5..d0e76e80b33 100644 --- a/hack/generator/pkg/astmodel/testdata/SetMapOfRequiredFromMapOfRequired.golden +++ b/hack/generator/pkg/astmodel/testdata/SetMapOfRequiredFromMapOfRequired.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // ratings ratingMap := make(map[string]int) @@ -26,7 +26,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // ratings ratingMap := make(map[string]int) diff --git a/hack/generator/pkg/astmodel/testdata/SetOptionalStringFromOptionalString.golden b/hack/generator/pkg/astmodel/testdata/SetOptionalStringFromOptionalString.golden index 02cdbfdf93d..c8c4c671fb0 100644 --- a/hack/generator/pkg/astmodel/testdata/SetOptionalStringFromOptionalString.golden +++ b/hack/generator/pkg/astmodel/testdata/SetOptionalStringFromOptionalString.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // name if source.name != nil { @@ -25,7 +25,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // name if person.name != nil { diff --git a/hack/generator/pkg/astmodel/testdata/SetOptionalStringFromString.golden b/hack/generator/pkg/astmodel/testdata/SetOptionalStringFromString.golden index e6b25a16f81..e434b99a6bc 100644 --- a/hack/generator/pkg/astmodel/testdata/SetOptionalStringFromString.golden +++ b/hack/generator/pkg/astmodel/testdata/SetOptionalStringFromString.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // name name := source.name @@ -21,7 +21,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // name if person.name != nil { diff --git a/hack/generator/pkg/astmodel/testdata/SetStringFromOptionalString.golden b/hack/generator/pkg/astmodel/testdata/SetStringFromOptionalString.golden index 0d6a47494fe..14163fd5aa9 100644 --- a/hack/generator/pkg/astmodel/testdata/SetStringFromOptionalString.golden +++ b/hack/generator/pkg/astmodel/testdata/SetStringFromOptionalString.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // name if source.name != nil { @@ -24,7 +24,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // name name := person.name diff --git a/hack/generator/pkg/astmodel/testdata/SetStringFromString.golden b/hack/generator/pkg/astmodel/testdata/SetStringFromString.golden index 6a16de0f0fc..61e578b5bfc 100644 --- a/hack/generator/pkg/astmodel/testdata/SetStringFromString.golden +++ b/hack/generator/pkg/astmodel/testdata/SetStringFromString.golden @@ -10,7 +10,7 @@ type Person struct { } // ConvertFromVNext populates our Person from the provided source Person -func (person Person) ConvertFromVNext(source *vNext.Person) error { +func (person *Person) ConvertFromVNext(source *vNext.Person) error { // name person.name = source.name @@ -20,7 +20,7 @@ func (person Person) ConvertFromVNext(source *vNext.Person) error { } // ConvertToVNext populates the provided destination Person from our Person -func (person Person) ConvertToVNext(destination *vNext.Person) error { +func (person *Person) ConvertToVNext(destination *vNext.Person) error { // name destination.name = person.name From e589430097045dea8083dcbadaca772054d389b1 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Tue, 11 May 2021 15:22:55 +1200 Subject: [PATCH 36/61] Pass address of local --- hack/generator/pkg/astmodel/storage_conversion_factories.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_factories.go b/hack/generator/pkg/astmodel/storage_conversion_factories.go index 3b9a7ed1032..7e06aefae92 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_factories.go +++ b/hack/generator/pkg/astmodel/storage_conversion_factories.go @@ -851,15 +851,18 @@ func assignObjectTypeFromObjectType( var conversion dst.Stmt if destinationName.PackageReference.Equals(generationContext.CurrentPackage()) { + // Destination is our current type conversion = astbuilder.SimpleAssignment( errLocal, tok, astbuilder.CallExpr(localId, conversionContext.functionName, actualReader)) + } else { + // Destination is another type conversion = astbuilder.SimpleAssignment( errLocal, tok, - astbuilder.CallExpr(reader, conversionContext.functionName, localId)) + astbuilder.CallExpr(reader, conversionContext.functionName, astbuilder.AddrOf(localId))) } checkForError := astbuilder.ReturnIfNotNil( From 717c8ce3f06aef3ff9433dfb31983d0ed6355fa4 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Thu, 13 May 2021 10:54:42 +1200 Subject: [PATCH 37/61] Exclude storage version types from hub version selection --- .../pkg/astmodel/package_reference.go | 16 ++++--- .../codegen/pipeline_mark_storage_version.go | 13 +++++- .../codegen/storage/storage_type_factory.go | 45 +++++++++++++++++++ 3 files changed, 67 insertions(+), 7 deletions(-) diff --git a/hack/generator/pkg/astmodel/package_reference.go b/hack/generator/pkg/astmodel/package_reference.go index 6875a4a0eca..6a246ced6a1 100644 --- a/hack/generator/pkg/astmodel/package_reference.go +++ b/hack/generator/pkg/astmodel/package_reference.go @@ -48,15 +48,19 @@ func PackageAsLocalPackage(pkg PackageReference) (LocalPackageReference, error) func SortPackageReferencesByPathAndVersion(packages []PackageReference) { sort.Slice(packages, func(i, j int) bool { - comparer := versionComparer{ - left: []rune(packages[i].PackagePath()), - right: []rune(packages[j].PackagePath()), - } - compare := comparer.Compare() - return compare < 0 + return ComparePathAndVersion(packages[i].PackagePath(), packages[j].PackagePath()) }) } +// ComparePathAndVersion compares two paths containing versions and returns true if left should go before right +func ComparePathAndVersion(left string, right string) bool { + comparer := versionComparer{ + left: []rune(left), + right: []rune(right), + } + return comparer.Compare() < 0 +} + // versionComparer captures our state while doing an alphanumeric version comparision // We need separate indexes for each side because we're doing a numeric comparison, which will // compare "100" and "0100" as equal (leading zeros are not significant) diff --git a/hack/generator/pkg/codegen/pipeline_mark_storage_version.go b/hack/generator/pkg/codegen/pipeline_mark_storage_version.go index bdb98bbadca..119bea869f6 100644 --- a/hack/generator/pkg/codegen/pipeline_mark_storage_version.go +++ b/hack/generator/pkg/codegen/pipeline_mark_storage_version.go @@ -72,6 +72,15 @@ func groupResourcesByVersion(types astmodel.Types) (map[unversionedName][]astmod result := make(map[unversionedName][]astmodel.TypeDefinition) for _, def := range types { + + // TODO: Remove this + // We want to explicitly avoid storage types for now, as we want to tag the latest API version to avoid a bug + // Once we get the full conversion functions in place, and a mapping that allows us to find correct API version + // within the controller, we can delete this + if astmodel.IsStoragePackageReference(def.Name().PackageReference) { + continue + } + if astmodel.IsResourceDefinition(def) { name, err := getUnversionedName(def.Name()) if err != nil { @@ -86,7 +95,9 @@ func groupResourcesByVersion(types astmodel.Types) (map[unversionedName][]astmod // order each set of resources by package name (== by version as these are sortable dates) for _, slice := range result { sort.Slice(slice, func(i, j int) bool { - return slice[i].Name().PackageReference.PackageName() < slice[j].Name().PackageReference.PackageName() + return astmodel.ComparePathAndVersion( + slice[i].Name().PackageReference.PackagePath(), + slice[j].Name().PackageReference.PackagePath()) }) } diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index c17355e8fd0..93af5e340d3 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -21,10 +21,12 @@ type StorageTypeFactory struct { propertyConversions []propertyConversion // Conversion rules to use for properties when creating storage variants pendingStorageConversion astmodel.TypeNameQueue // Queue of types that need storage variants created for them pendingConversionInjection astmodel.TypeNameQueue // Queue of types that need conversion functions injected + pendingMarkAsHubVersion astmodel.TypeNameQueue // Queue of types that need to be flagged as the hub storage version idFactory astmodel.IdentifierFactory // Factory for creating identifiers storageConverter astmodel.TypeVisitor // a cached type visitor used to create storage variants propertyConverter astmodel.TypeVisitor // a cached type visitor used to simplify property types functionInjector astmodel.TypeVisitor // a cached type visitor used to inject functions into definitions + resourceHubMarker astmodel.TypeVisitor // a cached type visitor used to mark resources as Storage Versions // Map of conversion links for creating our conversion graph // (Can't use PackageReferences as keys, so keyed by the full package path) @@ -38,6 +40,7 @@ func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) types: make(astmodel.Types), pendingStorageConversion: astmodel.MakeTypeNameQueue(), pendingConversionInjection: astmodel.MakeTypeNameQueue(), + pendingMarkAsHubVersion: astmodel.MakeTypeNameQueue(), idFactory: idFactory, conversionMap: make(map[string]astmodel.PackageReference), } @@ -66,6 +69,10 @@ func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) VisitResourceType: result.injectFunctionIntoResource, }.Build() + result.resourceHubMarker = astmodel.TypeVisitorBuilder{ + VisitResourceType: result.markResourceAsStorageVersion, + }.Build() + return result } @@ -98,6 +105,11 @@ func (f *StorageTypeFactory) process() error { return err } + err = f.pendingMarkAsHubVersion.Process(f.markAsHubVersion) + if err != nil { + return err + } + return nil } @@ -167,6 +179,9 @@ func (f *StorageTypeFactory) injectConversions(name astmodel.TypeName) error { if !ok { // No next package, so nothing to do // (this is expected if we have the hub storage package) + // Flag the type as needing to be flagged as the storage version + //TODO: Restore this - currently disabled until we get all the conversion functions injected + //!! f.pendingMarkAsHubVersion.Enqueue(name) return nil } @@ -208,6 +223,25 @@ func (f *StorageTypeFactory) injectConversions(name astmodel.TypeName) error { return nil } +func (f *StorageTypeFactory) markAsHubVersion(name astmodel.TypeName) error { + // Find the definition to modify + def, ok := f.types[name] + if !ok { + return errors.Errorf("failed to find definition for %q", name) + } + + // Mark the resource as the hub storage version + updated, err := f.resourceHubMarker.VisitDefinition(def, nil) + if err != nil { + return errors.Wrapf(err, "marking %q as hub storage version", name) + } + + // Update our map + f.types[name] = updated + + return nil +} + // makeStorageProperty applies a conversion to make a variant of the property for use when // serializing to storage func (f *StorageTypeFactory) makeStorageProperty( @@ -418,3 +452,14 @@ func (f *StorageTypeFactory) injectFunctionIntoResource( fn := ctx.(astmodel.Function) return rt.WithFunction(fn), nil } + +/* + * Functions used by the resourceHubMarker TypeVisitor + */ + +// injectFunctionIntoResource takes the function provided as a context and includes it on the +// provided resource type +func (f *StorageTypeFactory) markResourceAsStorageVersion( + _ *astmodel.TypeVisitor, rt *astmodel.ResourceType, ctx interface{}) (astmodel.Type, error) { + return rt.MarkAsStorageVersion(), nil +} From 0e0e788961161e3b99f89a084e5cd9cbe2860bbf Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 19 May 2021 13:03:16 +1200 Subject: [PATCH 38/61] Update from k8s-infra PR --- .../pkg/astmodel/known_locals_set.go | 1 + .../astmodel/storage_conversion_context.go | 7 ++-- .../astmodel/storage_conversion_factories.go | 13 +++---- .../astmodel/storage_conversion_function.go | 21 ++++++----- hack/generator/pkg/astmodel/type_visitor.go | 16 ++++----- .../codegen/pipeline_create_storage_types.go | 3 +- .../pipeline_ensure_type_has_arm_type.go | 6 ++-- .../codegen/storage/storage_type_factory.go | 36 +++++++++---------- 8 files changed, 52 insertions(+), 51 deletions(-) diff --git a/hack/generator/pkg/astmodel/known_locals_set.go b/hack/generator/pkg/astmodel/known_locals_set.go index 266e6c6c584..d65a94f4b0e 100644 --- a/hack/generator/pkg/astmodel/known_locals_set.go +++ b/hack/generator/pkg/astmodel/known_locals_set.go @@ -79,6 +79,7 @@ func (locals *KnownLocalsSet) Add(local string) { locals.names[name] = struct{}{} } +// HasName returns true if the specified name exists in the set, false otherwise func (locals *KnownLocalsSet) HasName(name string) bool { _, ok := locals.names[name] return ok diff --git a/hack/generator/pkg/astmodel/storage_conversion_context.go b/hack/generator/pkg/astmodel/storage_conversion_context.go index a6570a713b0..44da2a09b2f 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_context.go +++ b/hack/generator/pkg/astmodel/storage_conversion_context.go @@ -38,11 +38,10 @@ func (c *StorageConversionContext) WithKnownLocals(knownLocals *KnownLocalsSet) return result } -// NestedContext returns a nested context with independent knownLocals so that independent blocks -// can declare and reuse locals independently +// NestedContext returns a new context with a cloned knownLocals so that nested blocks +// can declare and reuse locals independently of each other. func (c *StorageConversionContext) NestedContext() *StorageConversionContext { result := c.clone() - result.knownLocals = c.knownLocals.Clone() return result } @@ -77,6 +76,6 @@ func (c *StorageConversionContext) clone() *StorageConversionContext { return &StorageConversionContext{ types: c.types, functionName: c.functionName, - knownLocals: c.knownLocals, + knownLocals: c.knownLocals.Clone(), } } diff --git a/hack/generator/pkg/astmodel/storage_conversion_factories.go b/hack/generator/pkg/astmodel/storage_conversion_factories.go index 7e06aefae92..e5dd57720de 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_factories.go +++ b/hack/generator/pkg/astmodel/storage_conversion_factories.go @@ -207,8 +207,9 @@ func assignFromOptionalType( } // Require a conversion between the unwrapped type and our source - // We supply a nested context as we will wrap the code generated by the conversion (if any) - // within a scope, making it independent of other code generated + // We supply a nested context as any code generated by the conversion will be within the if + // statement (a nested scope) and we don't want any variables declared in that scope to leak + // out elsewhere. unwrappedEndpoint := sourceEndpoint.WithType(sourceOptional.element) conversion, _ := createTypeConversion( unwrappedEndpoint, @@ -525,8 +526,8 @@ func assignArrayFromArray( } // Require a conversion between the array types - // We supply a nested context as we will wrap the code generated by the conversion (if any) - // within a loop, making it independent of other code generated + // We supply a nested context as any code generated by the conversion will be within the loop + // (a nested scope) and we don't want any variables declared in that scope to leak out elsewhere. unwrappedSourceEndpoint := sourceEndpoint.WithType(sourceArray.element) unwrappedDestinationEndpoint := destinationEndpoint.WithType(destinationArray.element) conversion, _ := createTypeConversion( @@ -609,8 +610,8 @@ func assignMapFromMap( } // Require a conversion between the map items - // We supply a nested context as we will wrap the code generated by the conversion (if any) - // within a loop, making it independent of other code generated + // We supply a nested context as any code generated by the conversion will be within the loop + // (a nested scope) and we don't want any variables declared in that scope to leak out elsewhere. unwrappedSourceEndpoint := sourceEndpoint.WithType(sourceMap.value) unwrappedDestinationEndpoint := destinationEndpoint.WithType(destinationMap.value) conversion, _ := createTypeConversion( diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index 73873f0872d..048347707c3 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -60,19 +60,19 @@ var _ Function = &StorageConversionFunction{} // NewStorageConversionFromFunction creates a new StorageConversionFunction to convert from the specified source func NewStorageConversionFromFunction( receiver TypeDefinition, - otherType TypeDefinition, + otherDefinition TypeDefinition, idFactory IdentifierFactory, conversionContext *StorageConversionContext, ) (*StorageConversionFunction, error) { result := &StorageConversionFunction{ - otherDefinition: otherType, + otherDefinition: otherDefinition, idFactory: idFactory, conversionDirection: ConvertFrom, conversions: make(map[string]StoragePropertyConversion), knownLocals: NewKnownLocalsSet(idFactory), } - version := idFactory.CreateIdentifier(otherType.Name().PackageReference.PackageName(), Exported) + version := idFactory.CreateIdentifier(otherDefinition.Name().PackageReference.PackageName(), Exported) result.name = "ConvertFrom" + version result.conversionContext = conversionContext.WithFunctionName(result.name).WithKnownLocals(result.knownLocals) @@ -87,19 +87,19 @@ func NewStorageConversionFromFunction( // NewStorageConversionToFunction creates a new StorageConversionFunction to convert to the specified destination func NewStorageConversionToFunction( receiver TypeDefinition, - otherType TypeDefinition, + otherDefinition TypeDefinition, idFactory IdentifierFactory, conversionContext *StorageConversionContext, ) (*StorageConversionFunction, error) { result := &StorageConversionFunction{ - otherDefinition: otherType, + otherDefinition: otherDefinition, idFactory: idFactory, conversionDirection: ConvertTo, conversions: make(map[string]StoragePropertyConversion), knownLocals: NewKnownLocalsSet(idFactory), } - version := idFactory.CreateIdentifier(otherType.Name().PackageReference.PackageName(), Exported) + version := idFactory.CreateIdentifier(otherDefinition.Name().PackageReference.PackageName(), Exported) result.name = "ConvertTo" + version result.conversionContext = conversionContext.WithFunctionName(result.name).WithKnownLocals(result.knownLocals) @@ -172,11 +172,15 @@ func (fn *StorageConversionFunction) AsFunc(generationContext *CodeGenerationCon panic(fmt.Sprintf("unexpected conversion direction %q", fn.conversionDirection)) } + // Create a sensible name for our receiver receiverName := fn.idFactory.CreateIdentifier(receiver.Name(), NotExported) + // We always use a pointer receiver so we can modify it + receiverType := NewOptionalType(receiver).AsType(generationContext) + funcDetails := &astbuilder.FuncDetails{ ReceiverIdent: receiverName, - ReceiverType: NewOptionalType(receiver).AsType(generationContext), + ReceiverType: receiverType, Name: fn.Name(), Body: fn.generateBody(receiverName, parameterName, generationContext), } @@ -350,7 +354,6 @@ func (fn *StorageConversionFunction) createConversions(receiver TypeDefinition, if len(properties) == 1 { label = "property" } - klog.V(2).Infof("Errors with %d %s", len(properties), label) return errors.Errorf("Errors with %d %s: %s", len(properties), label, strings.Join(properties, "; ")) } @@ -395,7 +398,7 @@ func (fn *StorageConversionFunction) createPropertyConversion( if err != nil { return nil, errors.Wrapf( err, - "trying to assign %q [%s] by converting from from %q [%s]", + "trying to assign %q [%s] by converting from %q [%s]", destinationProperty.PropertyName(), destinationProperty.PropertyType(), sourceProperty.PropertyName(), diff --git a/hack/generator/pkg/astmodel/type_visitor.go b/hack/generator/pkg/astmodel/type_visitor.go index f693341d5f1..b4f344efc92 100644 --- a/hack/generator/pkg/astmodel/type_visitor.go +++ b/hack/generator/pkg/astmodel/type_visitor.go @@ -178,12 +178,12 @@ func IdentityVisitOfObjectType(this *TypeVisitor, it *ObjectType, ctx interface{ func IdentityVisitOfMapType(this *TypeVisitor, it *MapType, ctx interface{}) (Type, error) { visitedKey, err := this.Visit(it.key, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit map key type %q", it.key.String()) + return nil, errors.Wrapf(err, "failed to visit map key type %q", it.key) } visitedValue, err := this.Visit(it.value, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit map value type %q", it.value.String()) + return nil, errors.Wrapf(err, "failed to visit map value type %q", it.value) } if visitedKey == it.key && visitedValue == it.value { @@ -202,7 +202,7 @@ func IdentityVisitOfEnumType(_ *TypeVisitor, it *EnumType, _ interface{}) (Type, func IdentityVisitOfOptionalType(this *TypeVisitor, it *OptionalType, ctx interface{}) (Type, error) { visitedElement, err := this.Visit(it.element, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit optional element type %q", it.element.String()) + return nil, errors.Wrapf(err, "failed to visit optional element type %q", it.element) } if visitedElement == it.element { @@ -215,12 +215,12 @@ func IdentityVisitOfOptionalType(this *TypeVisitor, it *OptionalType, ctx interf func IdentityVisitOfResourceType(this *TypeVisitor, it *ResourceType, ctx interface{}) (Type, error) { visitedSpec, err := this.Visit(it.spec, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit resource spec type %q", it.spec.String()) + return nil, errors.Wrapf(err, "failed to visit resource spec type %q", it.spec) } visitedStatus, err := this.Visit(it.status, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit resource status type %q", it.status.String()) + return nil, errors.Wrapf(err, "failed to visit resource status type %q", it.status) } if visitedSpec == it.spec && visitedStatus == it.status { @@ -296,7 +296,7 @@ func typeSlicesFastEqual(t1 []Type, t2 []Type) bool { func IdentityVisitOfFlaggedType(this *TypeVisitor, ft *FlaggedType, ctx interface{}) (Type, error) { nt, err := this.Visit(ft.element, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit flagged type %q", ft.element.String()) + return nil, errors.Wrapf(err, "failed to visit flagged type %q", ft.element) } if nt == ft.element { @@ -309,7 +309,7 @@ func IdentityVisitOfFlaggedType(this *TypeVisitor, ft *FlaggedType, ctx interfac func IdentityVisitOfValidatedType(this *TypeVisitor, v *ValidatedType, ctx interface{}) (Type, error) { nt, err := this.Visit(v.element, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit validated type %q", v.element.String()) + return nil, errors.Wrapf(err, "failed to visit validated type %q", v.element) } if nt == v.element { @@ -322,7 +322,7 @@ func IdentityVisitOfValidatedType(this *TypeVisitor, v *ValidatedType, ctx inter func IdentityVisitOfErroredType(this *TypeVisitor, e *ErroredType, ctx interface{}) (Type, error) { nt, err := this.Visit(e.inner, ctx) if err != nil { - return nil, errors.Wrapf(err, "failed to visit errored type %q", e.inner.String()) + return nil, errors.Wrapf(err, "failed to visit errored type %q", e.inner) } if nt == e.inner { diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index 126142a0679..e60e33459d3 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -40,8 +40,7 @@ func createStorageTypes(idFactory astmodel.IdentifierFactory) PipelineStage { factories[ref.Group()] = factory } - isArm := astmodel.ARMFlag.IsOn(def.Type()) - if isArm { + if astmodel.ARMFlag.IsOn(def.Type()) { // skip ARM types as they don't need storage variants continue } diff --git a/hack/generator/pkg/codegen/pipeline_ensure_type_has_arm_type.go b/hack/generator/pkg/codegen/pipeline_ensure_type_has_arm_type.go index 0f11df45405..47b911a566e 100644 --- a/hack/generator/pkg/codegen/pipeline_ensure_type_has_arm_type.go +++ b/hack/generator/pkg/codegen/pipeline_ensure_type_has_arm_type.go @@ -21,13 +21,13 @@ func ensureARMTypeExistsForEveryResource() PipelineStage { "ensureArmTypeExistsForEveryType", "Ensure that an ARM type for every top level resource spec/status exists", func(ctx context.Context, types astmodel.Types) (astmodel.Types, error) { - return types, validateAllTypesHaveARMType(types) + return types, validateExpectedTypesHaveARMType(types) }) } -// validateAllTypesHaveARMType returns an error containing details about all +// validateExpectedTypesHaveARMType returns an error containing details about all // types which do not have a matching ARM type. -func validateAllTypesHaveARMType(definitions astmodel.Types) error { +func validateExpectedTypesHaveARMType(definitions astmodel.Types) error { findARMType := func(t astmodel.Type) error { name, ok := astmodel.AsTypeName(t) if !ok { diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 93af5e340d3..ac955776b29 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -16,21 +16,18 @@ import ( // StorageTypeFactory is used to create storage types for a specific service type StorageTypeFactory struct { - service string // Name of the service we're handling (used mostly for logging) - types astmodel.Types // All the types for this service - propertyConversions []propertyConversion // Conversion rules to use for properties when creating storage variants - pendingStorageConversion astmodel.TypeNameQueue // Queue of types that need storage variants created for them - pendingConversionInjection astmodel.TypeNameQueue // Queue of types that need conversion functions injected - pendingMarkAsHubVersion astmodel.TypeNameQueue // Queue of types that need to be flagged as the hub storage version - idFactory astmodel.IdentifierFactory // Factory for creating identifiers - storageConverter astmodel.TypeVisitor // a cached type visitor used to create storage variants - propertyConverter astmodel.TypeVisitor // a cached type visitor used to simplify property types - functionInjector astmodel.TypeVisitor // a cached type visitor used to inject functions into definitions - resourceHubMarker astmodel.TypeVisitor // a cached type visitor used to mark resources as Storage Versions - - // Map of conversion links for creating our conversion graph - // (Can't use PackageReferences as keys, so keyed by the full package path) - conversionMap map[string]astmodel.PackageReference + service string // Name of the group we're handling (used mostly for logging) + types astmodel.Types // All the types for this group + propertyConversions []propertyConversion // Conversion rules to use for properties when creating storage variants + pendingStorageConversion astmodel.TypeNameQueue // Queue of types that need storage variants created for them + pendingConversionInjection astmodel.TypeNameQueue // Queue of types that need conversion functions injected + pendingMarkAsHubVersion astmodel.TypeNameQueue // Queue of types that need to be flagged as the hub storage version + idFactory astmodel.IdentifierFactory // Factory for creating identifiers + storageConverter astmodel.TypeVisitor // a cached type visitor used to create storage variants + propertyConverter astmodel.TypeVisitor // a cached type visitor used to simplify property types + functionInjector astmodel.TypeVisitor // a cached type visitor used to inject functions into definitions + resourceHubMarker astmodel.TypeVisitor // a cached type visitor used to mark resources as Storage Versions + conversionMap map[astmodel.PackageReference]astmodel.PackageReference // Map of conversion links for creating our conversion graph } // NewStorageTypeFactory creates a new instance of StorageTypeFactory ready for use @@ -42,7 +39,7 @@ func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) pendingConversionInjection: astmodel.MakeTypeNameQueue(), pendingMarkAsHubVersion: astmodel.MakeTypeNameQueue(), idFactory: idFactory, - conversionMap: make(map[string]astmodel.PackageReference), + conversionMap: make(map[astmodel.PackageReference]astmodel.PackageReference), } result.propertyConversions = []propertyConversion{ @@ -76,6 +73,7 @@ func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) return result } +// Add the supplied type definition to this factory func (f *StorageTypeFactory) Add(def astmodel.TypeDefinition) { f.types.Add(def) @@ -153,7 +151,7 @@ func (f *StorageTypeFactory) createStorageVariant(name astmodel.TypeName) error f.types.Add(storageDef) // Add API-Package -> Storage-Package link into the conversion map - f.conversionMap[name.PackageReference.PackagePath()] = storageDef.Name().PackageReference + f.conversionMap[name.PackageReference] = storageDef.Name().PackageReference // Queue for injection of conversion functions f.pendingConversionInjection.Enqueue(name) @@ -175,7 +173,7 @@ func (f *StorageTypeFactory) injectConversions(name astmodel.TypeName) error { } // Find the definition we want to convert to/from - nextPackage, ok := f.conversionMap[name.PackageReference.PackagePath()] + nextPackage, ok := f.conversionMap[name.PackageReference] if !ok { // No next package, so nothing to do // (this is expected if we have the hub storage package) @@ -460,6 +458,6 @@ func (f *StorageTypeFactory) injectFunctionIntoResource( // injectFunctionIntoResource takes the function provided as a context and includes it on the // provided resource type func (f *StorageTypeFactory) markResourceAsStorageVersion( - _ *astmodel.TypeVisitor, rt *astmodel.ResourceType, ctx interface{}) (astmodel.Type, error) { + _ *astmodel.TypeVisitor, rt *astmodel.ResourceType, _ interface{}) (astmodel.Type, error) { return rt.MarkAsStorageVersion(), nil } From dd25c46ae15bbb3c3c68df7e5b648f68f0bfffd5 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 19 May 2021 13:26:49 +1200 Subject: [PATCH 39/61] Create FunctionInjector abstraction --- .../pkg/codegen/storage/function_injector.go | 41 +++++++++++++++++++ .../codegen/storage/storage_type_factory.go | 33 ++------------- 2 files changed, 45 insertions(+), 29 deletions(-) create mode 100644 hack/generator/pkg/codegen/storage/function_injector.go diff --git a/hack/generator/pkg/codegen/storage/function_injector.go b/hack/generator/pkg/codegen/storage/function_injector.go new file mode 100644 index 00000000000..0390ec99696 --- /dev/null +++ b/hack/generator/pkg/codegen/storage/function_injector.go @@ -0,0 +1,41 @@ +package storage + +import "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" + +// FunctionInjector is a utility for injecting function definitions into resources and objects +type FunctionInjector struct { + visitor astmodel.TypeVisitor // Visitor used to achieve the required modification +} + +// NewFunctionInjector creates a new function injector for modifying resources and objects +func NewFunctionInjector() *FunctionInjector { + result := &FunctionInjector{} + + result.visitor = astmodel.TypeVisitorBuilder{ + VisitObjectType: result.injectFunctionIntoObject, + VisitResourceType: result.injectFunctionIntoResource, + }.Build() + + return result +} + +// Inject modifies the passed type definition by injecting the passed function +func (fi *FunctionInjector) Inject(def astmodel.TypeDefinition, fn astmodel.Function) (astmodel.TypeDefinition, error) { + return fi.visitor.VisitDefinition(def, fn) +} + +// injectFunctionIntoObject takes the function provided as a context and includes it on the +// provided object type +func (_ FunctionInjector) injectFunctionIntoObject( + _ *astmodel.TypeVisitor, ot *astmodel.ObjectType, ctx interface{}) (astmodel.Type, error) { + fn := ctx.(astmodel.Function) + return ot.WithFunction(fn), nil +} + +// injectFunctionIntoResource takes the function provided as a context and includes it on the +// provided resource type +func (_ FunctionInjector) injectFunctionIntoResource( + _ *astmodel.TypeVisitor, rt *astmodel.ResourceType, ctx interface{}) (astmodel.Type, error) { + fn := ctx.(astmodel.Function) + return rt.WithFunction(fn), nil +} diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index ac955776b29..eeba4deb615 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -25,7 +25,7 @@ type StorageTypeFactory struct { idFactory astmodel.IdentifierFactory // Factory for creating identifiers storageConverter astmodel.TypeVisitor // a cached type visitor used to create storage variants propertyConverter astmodel.TypeVisitor // a cached type visitor used to simplify property types - functionInjector astmodel.TypeVisitor // a cached type visitor used to inject functions into definitions + functionInjector *FunctionInjector // a utility used to inject functions into definitions resourceHubMarker astmodel.TypeVisitor // a cached type visitor used to mark resources as Storage Versions conversionMap map[astmodel.PackageReference]astmodel.PackageReference // Map of conversion links for creating our conversion graph } @@ -40,6 +40,7 @@ func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) pendingMarkAsHubVersion: astmodel.MakeTypeNameQueue(), idFactory: idFactory, conversionMap: make(map[astmodel.PackageReference]astmodel.PackageReference), + functionInjector: NewFunctionInjector(), } result.propertyConversions = []propertyConversion{ @@ -61,11 +62,6 @@ func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) VisitTypeName: result.shortCircuitNamesOfSimpleTypes, }.Build() - result.functionInjector = astmodel.TypeVisitorBuilder{ - VisitObjectType: result.injectFunctionIntoObject, - VisitResourceType: result.injectFunctionIntoResource, - }.Build() - result.resourceHubMarker = astmodel.TypeVisitorBuilder{ VisitResourceType: result.markResourceAsStorageVersion, }.Build() @@ -204,13 +200,12 @@ func (f *StorageTypeFactory) injectConversions(name astmodel.TypeName) error { return errors.Wrapf(err, "creating ConvertTo() function for %q", name) } - // Inject the functions into our type definition - def, err = f.functionInjector.VisitDefinition(def, convertFrom) + def, err = f.functionInjector.Inject(def, convertFrom) if err != nil { return errors.Wrapf(err, "failed to inject ConvertFrom function into %q", name) } - def, err = f.functionInjector.VisitDefinition(def, convertTo) + def, err = f.functionInjector.Inject(def, convertTo) if err != nil { return errors.Wrapf(err, "failed to inject ConvertFrom function into %q", name) } @@ -431,26 +426,6 @@ func (f *StorageTypeFactory) shortCircuitNamesOfSimpleTypes( return tv.Visit(actualType, ctx) } -/* - * Functions used by the functionInjector TypeVisitor - */ - -// injectFunctionIntoObject takes the function provided as a context and includes it on the -// provided object type -func (f *StorageTypeFactory) injectFunctionIntoObject( - _ *astmodel.TypeVisitor, ot *astmodel.ObjectType, ctx interface{}) (astmodel.Type, error) { - fn := ctx.(astmodel.Function) - return ot.WithFunction(fn), nil -} - -// injectFunctionIntoResource takes the function provided as a context and includes it on the -// provided resource type -func (f *StorageTypeFactory) injectFunctionIntoResource( - _ *astmodel.TypeVisitor, rt *astmodel.ResourceType, ctx interface{}) (astmodel.Type, error) { - fn := ctx.(astmodel.Function) - return rt.WithFunction(fn), nil -} - /* * Functions used by the resourceHubMarker TypeVisitor */ From 027d0444af3ddc39accd8bbe432ea130376c9395 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 19 May 2021 13:57:39 +1200 Subject: [PATCH 40/61] Create HubVersionMarker abstraction --- .../pkg/codegen/storage/hub_version_marker.go | 30 +++++++++++++++++++ .../codegen/storage/storage_type_factory.go | 20 ++----------- 2 files changed, 33 insertions(+), 17 deletions(-) create mode 100644 hack/generator/pkg/codegen/storage/hub_version_marker.go diff --git a/hack/generator/pkg/codegen/storage/hub_version_marker.go b/hack/generator/pkg/codegen/storage/hub_version_marker.go new file mode 100644 index 00000000000..0be8ecc85c1 --- /dev/null +++ b/hack/generator/pkg/codegen/storage/hub_version_marker.go @@ -0,0 +1,30 @@ +package storage + +import "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" + +// HubVersionMarker is a utility for marking resource types as "hub" versions +type HubVersionMarker struct { + visitor astmodel.TypeVisitor // Visitor used to achieve the required modification +} + +// NewHubVersionMarker returns a new hub version marker for flagging resource types +func NewHubVersionMarker() *HubVersionMarker { + result := &HubVersionMarker{} + + result.visitor = astmodel.TypeVisitorBuilder{ + VisitResourceType: result.markResourceAsStorageVersion, + }.Build() + + return result +} + +// MarkAsStorageVersion mark the supplied type definition as the storage version +func (m *HubVersionMarker) MarkAsStorageVersion(def astmodel.TypeDefinition) (astmodel.TypeDefinition, error) { + return m.visitor.VisitDefinition(def, nil) +} + +// markResourceAsStorageVersion marks the supplied resource as the canonical hub (storage) version +func (m *HubVersionMarker) markResourceAsStorageVersion( + _ *astmodel.TypeVisitor, rt *astmodel.ResourceType, _ interface{}) (astmodel.Type, error) { + return rt.MarkAsStorageVersion(), nil +} diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index eeba4deb615..2696bb68e24 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -26,7 +26,7 @@ type StorageTypeFactory struct { storageConverter astmodel.TypeVisitor // a cached type visitor used to create storage variants propertyConverter astmodel.TypeVisitor // a cached type visitor used to simplify property types functionInjector *FunctionInjector // a utility used to inject functions into definitions - resourceHubMarker astmodel.TypeVisitor // a cached type visitor used to mark resources as Storage Versions + resourceHubMarker *HubVersionMarker // a utility used to mark resources as Storage Versions conversionMap map[astmodel.PackageReference]astmodel.PackageReference // Map of conversion links for creating our conversion graph } @@ -41,6 +41,7 @@ func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) idFactory: idFactory, conversionMap: make(map[astmodel.PackageReference]astmodel.PackageReference), functionInjector: NewFunctionInjector(), + resourceHubMarker: NewHubVersionMarker(), } result.propertyConversions = []propertyConversion{ @@ -62,10 +63,6 @@ func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) VisitTypeName: result.shortCircuitNamesOfSimpleTypes, }.Build() - result.resourceHubMarker = astmodel.TypeVisitorBuilder{ - VisitResourceType: result.markResourceAsStorageVersion, - }.Build() - return result } @@ -224,7 +221,7 @@ func (f *StorageTypeFactory) markAsHubVersion(name astmodel.TypeName) error { } // Mark the resource as the hub storage version - updated, err := f.resourceHubMarker.VisitDefinition(def, nil) + updated, err := f.resourceHubMarker.MarkAsStorageVersion(def) if err != nil { return errors.Wrapf(err, "marking %q as hub storage version", name) } @@ -425,14 +422,3 @@ func (f *StorageTypeFactory) shortCircuitNamesOfSimpleTypes( // Replace the name with the underlying type return tv.Visit(actualType, ctx) } - -/* - * Functions used by the resourceHubMarker TypeVisitor - */ - -// injectFunctionIntoResource takes the function provided as a context and includes it on the -// provided resource type -func (f *StorageTypeFactory) markResourceAsStorageVersion( - _ *astmodel.TypeVisitor, rt *astmodel.ResourceType, _ interface{}) (astmodel.Type, error) { - return rt.MarkAsStorageVersion(), nil -} From 00884b39f7e771f424497f4afa83e7300bf1866f Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 19 May 2021 14:34:56 +1200 Subject: [PATCH 41/61] Create PropertyConverter abstraction --- .../pkg/codegen/storage/property_converter.go | 141 ++++++++++++++++++ .../codegen/storage/storage_type_factory.go | 109 +------------- 2 files changed, 143 insertions(+), 107 deletions(-) create mode 100644 hack/generator/pkg/codegen/storage/property_converter.go diff --git a/hack/generator/pkg/codegen/storage/property_converter.go b/hack/generator/pkg/codegen/storage/property_converter.go new file mode 100644 index 00000000000..58da61c8b14 --- /dev/null +++ b/hack/generator/pkg/codegen/storage/property_converter.go @@ -0,0 +1,141 @@ +package storage + +import ( + "fmt" + "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" + "github.com/pkg/errors" +) + +// PropertyConverter is used to convert the properties of object types as required for storage variants +type PropertyConverter struct { + visitor astmodel.TypeVisitor // Visitor used to achieve the required modification + types astmodel.Types // All the types for this group + propertyConversions []propertyConversion // Conversion rules to use for properties when creating storage variants +} + +// NewPropertyConverter creates a new property converter for modifying object properties +func NewPropertyConverter(types astmodel.Types) *PropertyConverter { + result := &PropertyConverter{ + types: types, + } + + result.propertyConversions = []propertyConversion{ + result.preserveKubernetesResourceStorageProperties, + result.defaultPropertyConversion, + } + + result.visitor = astmodel.TypeVisitorBuilder{ + VisitEnumType: result.useBaseTypeForEnumerations, + VisitValidatedType: result.stripAllValidations, + VisitTypeName: result.shortCircuitNamesOfSimpleTypes, + }.Build() + + return result +} + +// ConvertProperty applies our conversion rules to a specific property +func (p PropertyConverter) ConvertProperty(property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { + + for _, conv := range p.propertyConversions { + prop, err := conv(property) + if err != nil { + // Something went wrong, return the error + return nil, err + } + if prop != nil { + // We have the conversion we need, return it promptly + return prop, nil + } + } + + return nil, fmt.Errorf("failed to find a conversion for property %v", property.PropertyName()) +} + +// stripAllValidations removes all validations +func (p PropertyConverter) stripAllValidations( + this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { + // strip all type validations from storage property types + // act as if they do not exist + return this.Visit(v.ElementType(), ctx) +} + +// useBaseTypeForEnumerations replaces an enumeration with its underlying base type +func (p PropertyConverter) useBaseTypeForEnumerations( + tv *astmodel.TypeVisitor, et *astmodel.EnumType, ctx interface{}) (astmodel.Type, error) { + return tv.Visit(et.BaseType(), ctx) +} + +// shortCircuitNamesOfSimpleTypes redirects TypeNames that reference resources or objects into our +// storage namespace, and replaces TypeNames that point to simple types (enumerations or +// primitives) with their underlying type. +func (p PropertyConverter) shortCircuitNamesOfSimpleTypes( + tv *astmodel.TypeVisitor, tn astmodel.TypeName, ctx interface{}) (astmodel.Type, error) { + + actualType, err := p.types.FullyResolve(tn) + if err != nil { + // Can't resolve to underlying type, give up + return nil, err + } + + _, isObject := astmodel.AsObjectType(actualType) + _, isResource := astmodel.AsResourceType(actualType) + + if isObject || isResource { + // We have an object or a resource, redirect to our storage namespace if we can + if storageName, ok := p.tryConvertToStorageNamespace(tn); ok { + return storageName, nil + } + + // Otherwise just keep the name + return tn, nil + } + + // Replace the name with the underlying type + return tv.Visit(actualType, ctx) +} + +func (_ PropertyConverter) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { + // Map the type name into our storage namespace + localRef, ok := name.PackageReference.AsLocalPackage() + if !ok { + return astmodel.TypeName{}, false + } + + storageRef := astmodel.MakeStoragePackageReference(localRef) + visitedName := astmodel.MakeTypeName(storageRef, name.Name()) + return visitedName, true +} + +// A property conversion accepts a property definition and optionally applies a conversion to make +// the property suitable for use on a storage type. Conversions return nil if they decline to +// convert, deferring the conversion to another. +type propertyConversion = func(property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) + +// preserveKubernetesResourceStorageProperties preserves properties required by the +// KubernetesResource interface as they're always required exactly as declared +func (p PropertyConverter) preserveKubernetesResourceStorageProperties( + prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { + + if astmodel.IsKubernetesResourceProperty(prop.PropertyName()) { + // Keep these unchanged + return prop, nil + } + + // Not a kubernetes type, defer to another conversion + return nil, nil +} + +func (p PropertyConverter) defaultPropertyConversion( + property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { + + propertyType, err := p.visitor.Visit(property.PropertyType(), nil) + if err != nil { + return nil, errors.Wrapf(err, "converting property %q", property.PropertyName()) + } + + newProperty := property.WithType(propertyType). + MakeOptional(). + WithDescription("") + + return newProperty, nil +} diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 2696bb68e24..900da86bdd7 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -18,13 +18,12 @@ import ( type StorageTypeFactory struct { service string // Name of the group we're handling (used mostly for logging) types astmodel.Types // All the types for this group - propertyConversions []propertyConversion // Conversion rules to use for properties when creating storage variants pendingStorageConversion astmodel.TypeNameQueue // Queue of types that need storage variants created for them pendingConversionInjection astmodel.TypeNameQueue // Queue of types that need conversion functions injected pendingMarkAsHubVersion astmodel.TypeNameQueue // Queue of types that need to be flagged as the hub storage version idFactory astmodel.IdentifierFactory // Factory for creating identifiers storageConverter astmodel.TypeVisitor // a cached type visitor used to create storage variants - propertyConverter astmodel.TypeVisitor // a cached type visitor used to simplify property types + propertyConverter *PropertyConverter // a utility used to simplify property types functionInjector *FunctionInjector // a utility used to inject functions into definitions resourceHubMarker *HubVersionMarker // a utility used to mark resources as Storage Versions conversionMap map[astmodel.PackageReference]astmodel.PackageReference // Map of conversion links for creating our conversion graph @@ -44,10 +43,7 @@ func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) resourceHubMarker: NewHubVersionMarker(), } - result.propertyConversions = []propertyConversion{ - result.preserveKubernetesResourceStorageProperties, - result.defaultPropertyConversion, - } + result.propertyConverter = NewPropertyConverter(result.types) result.storageConverter = astmodel.TypeVisitorBuilder{ VisitObjectType: result.convertObjectType, @@ -57,12 +53,6 @@ func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) VisitFlaggedType: result.stripAllFlags, }.Build() - result.propertyConverter = astmodel.TypeVisitorBuilder{ - VisitEnumType: result.useBaseTypeForEnumerations, - VisitValidatedType: result.stripAllValidations, - VisitTypeName: result.shortCircuitNamesOfSimpleTypes, - }.Build() - return result } @@ -232,30 +222,6 @@ func (f *StorageTypeFactory) markAsHubVersion(name astmodel.TypeName) error { return nil } -// makeStorageProperty applies a conversion to make a variant of the property for use when -// serializing to storage -func (f *StorageTypeFactory) makeStorageProperty( - prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { - for _, conv := range f.propertyConversions { - p, err := conv(prop) - if err != nil { - // Something went wrong, return the error - return nil, err - } - if p != nil { - // We have the conversion we need, return it promptly - return p, nil - } - } - - return nil, fmt.Errorf("failed to find a conversion for property %v", prop.PropertyName()) -} - -// A property conversion accepts a property definition and optionally applies a conversion to make -// the property suitable for use on a storage type. Conversions return nil if they decline to -// convert, deferring the conversion to another. -type propertyConversion = func(property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) - // descriptionForStorageVariant creates a description for a storage variant, indicating which // original type it is based upon func (f *StorageTypeFactory) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { @@ -281,38 +247,6 @@ func (f *StorageTypeFactory) tryConvertToStorageNamespace(name astmodel.TypeName return visitedName, true } -/* - * Functions used as propertyConversions - */ - -// preserveKubernetesResourceStorageProperties preserves properties required by the -// KubernetesResource interface as they're always required exactly as declared -func (f *StorageTypeFactory) preserveKubernetesResourceStorageProperties( - prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { - - if astmodel.IsKubernetesResourceProperty(prop.PropertyName()) { - // Keep these unchanged - return prop, nil - } - - // Not a kubernetes type, defer to another conversion - return nil, nil -} - -func (f *StorageTypeFactory) defaultPropertyConversion( - prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { - propertyType, err := f.propertyConverter.Visit(prop.PropertyType(), nil) - if err != nil { - return nil, err - } - - p := prop.WithType(propertyType). - MakeOptional(). - WithDescription("") - - return p, nil -} - /* * Functions used by the storageConverter TypeVisitor */ @@ -383,42 +317,3 @@ func (f *StorageTypeFactory) stripAllFlags( return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) } - -/* - * Functions used by the propertyConverter TypeVisitor - */ - -// useBaseTypeForEnumerations replaces an enumeration with its underlying base type -func (f *StorageTypeFactory) useBaseTypeForEnumerations( - tv *astmodel.TypeVisitor, et *astmodel.EnumType, ctx interface{}) (astmodel.Type, error) { - return tv.Visit(et.BaseType(), ctx) -} - -// shortCircuitNamesOfSimpleTypes redirects TypeNames that reference resources or objects into our -// storage namespace, and replaces TypeNames that point to simple types (enumerations or -// primitives) with their underlying type. -func (f *StorageTypeFactory) shortCircuitNamesOfSimpleTypes( - tv *astmodel.TypeVisitor, tn astmodel.TypeName, ctx interface{}) (astmodel.Type, error) { - - actualType, err := f.types.FullyResolve(tn) - if err != nil { - // Can't resolve to underlying type, give up - return nil, err - } - - _, isObject := astmodel.AsObjectType(actualType) - _, isResource := astmodel.AsResourceType(actualType) - - if isObject || isResource { - // We have an object or a resource, redirect to our storage namespace if we can - if storageName, ok := f.tryConvertToStorageNamespace(tn); ok { - return storageName, nil - } - - // Otherwise just keep the name - return tn, nil - } - - // Replace the name with the underlying type - return tv.Visit(actualType, ctx) -} From f2b15df549ab99f21f97894a05325aa0a1674f80 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 19 May 2021 15:03:25 +1200 Subject: [PATCH 42/61] Create TypeConverter abstraction --- .../codegen/storage/storage_type_factory.go | 117 +-------------- .../pkg/codegen/storage/type_converter.go | 142 ++++++++++++++++++ 2 files changed, 145 insertions(+), 114 deletions(-) create mode 100644 hack/generator/pkg/codegen/storage/type_converter.go diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 900da86bdd7..e02821fed36 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -6,11 +6,8 @@ package storage import ( - "fmt" - "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" "github.com/pkg/errors" - kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/klog/v2" ) @@ -22,8 +19,7 @@ type StorageTypeFactory struct { pendingConversionInjection astmodel.TypeNameQueue // Queue of types that need conversion functions injected pendingMarkAsHubVersion astmodel.TypeNameQueue // Queue of types that need to be flagged as the hub storage version idFactory astmodel.IdentifierFactory // Factory for creating identifiers - storageConverter astmodel.TypeVisitor // a cached type visitor used to create storage variants - propertyConverter *PropertyConverter // a utility used to simplify property types + typeConverter *TypeConverter // a utility type type visitor used to create storage variants functionInjector *FunctionInjector // a utility used to inject functions into definitions resourceHubMarker *HubVersionMarker // a utility used to mark resources as Storage Versions conversionMap map[astmodel.PackageReference]astmodel.PackageReference // Map of conversion links for creating our conversion graph @@ -43,15 +39,7 @@ func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) resourceHubMarker: NewHubVersionMarker(), } - result.propertyConverter = NewPropertyConverter(result.types) - - result.storageConverter = astmodel.TypeVisitorBuilder{ - VisitObjectType: result.convertObjectType, - VisitResourceType: result.convertResourceType, - VisitTypeName: result.redirectTypeNamesToStoragePackage, - VisitValidatedType: result.stripAllValidations, - VisitFlaggedType: result.stripAllFlags, - }.Build() + result.typeConverter = NewTypeConverter(result.types) return result } @@ -123,14 +111,11 @@ func (f *StorageTypeFactory) createStorageVariant(name astmodel.TypeName) error return errors.Errorf("failed to find definition for %q", name) } - storageDef, err := f.storageConverter.VisitDefinition(def, nil) + storageDef, err := f.typeConverter.ConvertDefinition(def) if err != nil { return errors.Wrapf(err, "creating storage variant for %q", name) } - desc := f.descriptionForStorageVariant(def) - storageDef = storageDef.WithDescription(desc) - f.types.Add(storageDef) // Add API-Package -> Storage-Package link into the conversion map @@ -221,99 +206,3 @@ func (f *StorageTypeFactory) markAsHubVersion(name astmodel.TypeName) error { return nil } - -// descriptionForStorageVariant creates a description for a storage variant, indicating which -// original type it is based upon -func (f *StorageTypeFactory) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { - pkg := definition.Name().PackageReference.PackageName() - - result := []string{ - fmt.Sprintf("Storage version of %v.%v", pkg, definition.Name().Name()), - } - result = append(result, definition.Description()...) - - return result -} - -func (f *StorageTypeFactory) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { - // Map the type name into our storage namespace - localRef, ok := name.PackageReference.AsLocalPackage() - if !ok { - return astmodel.TypeName{}, false - } - - storageRef := astmodel.MakeStoragePackageReference(localRef) - visitedName := astmodel.MakeTypeName(storageRef, name.Name()) - return visitedName, true -} - -/* - * Functions used by the storageConverter TypeVisitor - */ - -// convertResourceType creates a storage variation of a resource type -func (f *StorageTypeFactory) convertResourceType( - tv *astmodel.TypeVisitor, - resource *astmodel.ResourceType, - ctx interface{}) (astmodel.Type, error) { - - // storage resource types do not need defaulter interface, they have no webhooks - rsrc := resource.WithoutInterface(astmodel.DefaulterInterfaceName) - - return astmodel.IdentityVisitOfResourceType(tv, rsrc, ctx) -} - -// convertObjectType creates a storage variation of an object type -func (f *StorageTypeFactory) convertObjectType( - _ *astmodel.TypeVisitor, object *astmodel.ObjectType, _ interface{}) (astmodel.Type, error) { - - var errs []error - properties := object.Properties() - for i, prop := range properties { - p, err := f.makeStorageProperty(prop) - if err != nil { - errs = append(errs, errors.Wrapf(err, "property %s", prop.PropertyName())) - } else { - properties[i] = p - } - } - - if len(errs) > 0 { - err := kerrors.NewAggregate(errs) - return nil, err - } - - objectType := astmodel.NewObjectType().WithProperties(properties...) - return astmodel.StorageFlag.ApplyTo(objectType), nil -} - -// redirectTypeNamesToStoragePackage modifies TypeNames to reference the current storage package -func (f *StorageTypeFactory) redirectTypeNamesToStoragePackage( - _ *astmodel.TypeVisitor, name astmodel.TypeName, _ interface{}) (astmodel.Type, error) { - if result, ok := f.tryConvertToStorageNamespace(name); ok { - return result, nil - } - - return name, nil -} - -// stripAllValidations removes all validations -func (f *StorageTypeFactory) stripAllValidations( - this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { - // strip all type validations from storage types, - // act as if they do not exist - return this.Visit(v.ElementType(), ctx) -} - -// stripAllFlags removes all flags -func (f *StorageTypeFactory) stripAllFlags( - tv *astmodel.TypeVisitor, - flaggedType *astmodel.FlaggedType, - ctx interface{}) (astmodel.Type, error) { - if flaggedType.HasFlag(astmodel.ARMFlag) { - // We don't want to do anything with ARM types - return flaggedType, nil - } - - return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) -} diff --git a/hack/generator/pkg/codegen/storage/type_converter.go b/hack/generator/pkg/codegen/storage/type_converter.go new file mode 100644 index 00000000000..6d83e0517ad --- /dev/null +++ b/hack/generator/pkg/codegen/storage/type_converter.go @@ -0,0 +1,142 @@ +package storage + +import ( + "fmt" + "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" + "github.com/pkg/errors" + kerrors "k8s.io/apimachinery/pkg/util/errors" +) + +// TypeConverter is used to create a storage variant of an API type +type TypeConverter struct { + visitor astmodel.TypeVisitor // Visitor used to achieve the required modification + types astmodel.Types // All the types for this group + propertyConverter *PropertyConverter // nested converter for properties +} + +// NewTypeConverter creates a new instance of the utility type +func NewTypeConverter(types astmodel.Types) *TypeConverter { + result := &TypeConverter{ + types: types, + propertyConverter: NewPropertyConverter(types), + } + + result.visitor = astmodel.TypeVisitorBuilder{ + VisitObjectType: result.convertObjectType, + VisitResourceType: result.convertResourceType, + VisitTypeName: result.redirectTypeNamesToStoragePackage, + VisitValidatedType: result.stripAllValidations, + VisitFlaggedType: result.stripAllFlags, + }.Build() + + return result +} + +// ConvertDefinition applies our type conversion to a specific type definition +func (t TypeConverter) ConvertDefinition(def astmodel.TypeDefinition) (astmodel.TypeDefinition, error) { + result, err := t.visitor.VisitDefinition(def, nil) + if err != nil { + return astmodel.TypeDefinition{}, errors.Wrapf(err, "converting %q for storage variant", def.Name()) + } + + description := t.descriptionForStorageVariant(def) + result = result.WithDescription(description) + + return result, nil +} + +/* + * Functions used by the typeConverter TypeVisitor + */ + +// convertResourceType creates a storage variation of a resource type +func (t TypeConverter) convertResourceType( + tv *astmodel.TypeVisitor, + resource *astmodel.ResourceType, + ctx interface{}) (astmodel.Type, error) { + + // storage resource types do not need defaulter interface, they have no webhooks + modifiedResource := resource.WithoutInterface(astmodel.DefaulterInterfaceName) + + return astmodel.IdentityVisitOfResourceType(tv, modifiedResource, ctx) +} + +// convertObjectType creates a storage variation of an object type +func (t TypeConverter) convertObjectType( + _ *astmodel.TypeVisitor, object *astmodel.ObjectType, _ interface{}) (astmodel.Type, error) { + + var errs []error + properties := object.Properties() + for i, prop := range properties { + p, err := t.propertyConverter.ConvertProperty(prop) + if err != nil { + errs = append(errs, errors.Wrapf(err, "property %s", prop.PropertyName())) + } else { + properties[i] = p + } + } + + if len(errs) > 0 { + err := kerrors.NewAggregate(errs) + return nil, err + } + + objectType := astmodel.NewObjectType().WithProperties(properties...) + return astmodel.StorageFlag.ApplyTo(objectType), nil +} + +// redirectTypeNamesToStoragePackage modifies TypeNames to reference the current storage package +func (t TypeConverter) redirectTypeNamesToStoragePackage( + _ *astmodel.TypeVisitor, name astmodel.TypeName, _ interface{}) (astmodel.Type, error) { + if result, ok := t.tryConvertToStorageNamespace(name); ok { + return result, nil + } + + return name, nil +} + +// stripAllValidations removes all validations +func (t TypeConverter) stripAllValidations( + this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { + // strip all type validations from storage types, + // act as if they do not exist + return this.Visit(v.ElementType(), ctx) +} + +// stripAllFlags removes all flags +func (t TypeConverter) stripAllFlags( + tv *astmodel.TypeVisitor, + flaggedType *astmodel.FlaggedType, + ctx interface{}) (astmodel.Type, error) { + if flaggedType.HasFlag(astmodel.ARMFlag) { + // We don't want to do anything with ARM types + return flaggedType, nil + } + + return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) +} + +func (_ TypeConverter) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { + // Map the type name into our storage namespace + localRef, ok := name.PackageReference.AsLocalPackage() + if !ok { + return astmodel.TypeName{}, false + } + + storageRef := astmodel.MakeStoragePackageReference(localRef) + visitedName := astmodel.MakeTypeName(storageRef, name.Name()) + return visitedName, true +} + +// descriptionForStorageVariant creates a description for a storage variant, indicating which +// original type it is based upon +func (_ TypeConverter) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { + pkg := definition.Name().PackageReference.PackageName() + + result := []string{ + fmt.Sprintf("Storage version of %v.%v", pkg, definition.Name().Name()), + } + result = append(result, definition.Description()...) + + return result +} From e635b875a38517ac48097b7e5219af45a5851581 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 19 May 2021 15:21:01 +1200 Subject: [PATCH 43/61] Add required headers --- hack/generator/pkg/codegen/storage/function_injector.go | 5 +++++ hack/generator/pkg/codegen/storage/hub_version_marker.go | 5 +++++ hack/generator/pkg/codegen/storage/property_converter.go | 5 +++++ hack/generator/pkg/codegen/storage/type_converter.go | 5 +++++ 4 files changed, 20 insertions(+) diff --git a/hack/generator/pkg/codegen/storage/function_injector.go b/hack/generator/pkg/codegen/storage/function_injector.go index 0390ec99696..d636a80a8c6 100644 --- a/hack/generator/pkg/codegen/storage/function_injector.go +++ b/hack/generator/pkg/codegen/storage/function_injector.go @@ -1,3 +1,8 @@ +/* + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT license. + */ + package storage import "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" diff --git a/hack/generator/pkg/codegen/storage/hub_version_marker.go b/hack/generator/pkg/codegen/storage/hub_version_marker.go index 0be8ecc85c1..75c1d8d7329 100644 --- a/hack/generator/pkg/codegen/storage/hub_version_marker.go +++ b/hack/generator/pkg/codegen/storage/hub_version_marker.go @@ -1,3 +1,8 @@ +/* + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT license. + */ + package storage import "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" diff --git a/hack/generator/pkg/codegen/storage/property_converter.go b/hack/generator/pkg/codegen/storage/property_converter.go index 58da61c8b14..18f2ab6a0f5 100644 --- a/hack/generator/pkg/codegen/storage/property_converter.go +++ b/hack/generator/pkg/codegen/storage/property_converter.go @@ -1,3 +1,8 @@ +/* + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT license. + */ + package storage import ( diff --git a/hack/generator/pkg/codegen/storage/type_converter.go b/hack/generator/pkg/codegen/storage/type_converter.go index 6d83e0517ad..e0e20073cf1 100644 --- a/hack/generator/pkg/codegen/storage/type_converter.go +++ b/hack/generator/pkg/codegen/storage/type_converter.go @@ -1,3 +1,8 @@ +/* + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT license. + */ + package storage import ( From c0c74cb9ca84711942ff8ca55384c61a642d7080 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 19 May 2021 16:08:01 +1200 Subject: [PATCH 44/61] Fix references --- hack/generator/pkg/codegen/storage/function_injector.go | 2 +- hack/generator/pkg/codegen/storage/hub_version_marker.go | 2 +- hack/generator/pkg/codegen/storage/property_converter.go | 2 +- hack/generator/pkg/codegen/storage/storage_type_factory.go | 2 +- hack/generator/pkg/codegen/storage/type_converter.go | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/function_injector.go b/hack/generator/pkg/codegen/storage/function_injector.go index d636a80a8c6..f772c1ff17d 100644 --- a/hack/generator/pkg/codegen/storage/function_injector.go +++ b/hack/generator/pkg/codegen/storage/function_injector.go @@ -5,7 +5,7 @@ package storage -import "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" +import "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" // FunctionInjector is a utility for injecting function definitions into resources and objects type FunctionInjector struct { diff --git a/hack/generator/pkg/codegen/storage/hub_version_marker.go b/hack/generator/pkg/codegen/storage/hub_version_marker.go index 75c1d8d7329..58f3e41905d 100644 --- a/hack/generator/pkg/codegen/storage/hub_version_marker.go +++ b/hack/generator/pkg/codegen/storage/hub_version_marker.go @@ -5,7 +5,7 @@ package storage -import "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" +import "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" // HubVersionMarker is a utility for marking resource types as "hub" versions type HubVersionMarker struct { diff --git a/hack/generator/pkg/codegen/storage/property_converter.go b/hack/generator/pkg/codegen/storage/property_converter.go index 18f2ab6a0f5..949cb592111 100644 --- a/hack/generator/pkg/codegen/storage/property_converter.go +++ b/hack/generator/pkg/codegen/storage/property_converter.go @@ -7,7 +7,7 @@ package storage import ( "fmt" - "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" + "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" "github.com/pkg/errors" ) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index e02821fed36..f3a1a254e1b 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -6,7 +6,7 @@ package storage import ( - "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" + "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" "github.com/pkg/errors" "k8s.io/klog/v2" ) diff --git a/hack/generator/pkg/codegen/storage/type_converter.go b/hack/generator/pkg/codegen/storage/type_converter.go index e0e20073cf1..e90e9a67058 100644 --- a/hack/generator/pkg/codegen/storage/type_converter.go +++ b/hack/generator/pkg/codegen/storage/type_converter.go @@ -7,7 +7,7 @@ package storage import ( "fmt" - "github.com/Azure/k8s-infra/hack/generator/pkg/astmodel" + "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" "github.com/pkg/errors" kerrors "k8s.io/apimachinery/pkg/util/errors" ) From 97c712f54fd2b8bdc726851ae10cbe6c5f8c1c20 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 24 May 2021 08:00:14 +1200 Subject: [PATCH 45/61] go fmt --- hack/generator/pkg/codegen/pipeline_create_storage_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index e60e33459d3..548d4c907d7 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -9,8 +9,8 @@ import ( "context" "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" "github.com/Azure/azure-service-operator/hack/generator/pkg/codegen/storage" - "k8s.io/klog/v2" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/klog/v2" ) // createStorageTypes returns a pipeline stage that creates dedicated storage types for each resource and nested object. From 7d7b08f9435c62c91e9e9e06c3e06fe083e09dce Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 24 May 2021 09:00:28 +1200 Subject: [PATCH 46/61] Fix bug with missing KnownLocalsSet in context --- hack/generator/pkg/astmodel/storage_conversion_context.go | 5 +++-- .../pkg/astmodel/storage_conversion_function_test.go | 2 +- .../ConvertBetweenOptionalObjectAndOptionalObject.golden | 2 +- .../ConvertBetweenRequiredObjectAndOptionalObject.golden | 2 +- hack/generator/pkg/codegen/storage/storage_type_factory.go | 2 +- 5 files changed, 7 insertions(+), 6 deletions(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_context.go b/hack/generator/pkg/astmodel/storage_conversion_context.go index 44da2a09b2f..7c60ca449d4 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_context.go +++ b/hack/generator/pkg/astmodel/storage_conversion_context.go @@ -13,14 +13,15 @@ type StorageConversionContext struct { // functionName is the name of the function we're currently generating functionName string // knownLocals is a reference to our set of local variables + // (Pointer because it's a reference type, not because it's optional) knownLocals *KnownLocalsSet } // NewStorageConversionContext creates a new instance of a StorageConversionContext -func NewStorageConversionContext(types Types) *StorageConversionContext { +func NewStorageConversionContext(types Types, idFactory IdentifierFactory) *StorageConversionContext { return &StorageConversionContext{ types: types, - knownLocals: nil, + knownLocals: NewKnownLocalsSet(idFactory), } } diff --git a/hack/generator/pkg/astmodel/storage_conversion_function_test.go b/hack/generator/pkg/astmodel/storage_conversion_function_test.go index 3c3a60e046e..ba5740668bb 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function_test.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function_test.go @@ -167,7 +167,7 @@ func RunTestStorageConversionFunction_AsFunc(c *StorageConversionPropertyTestCas idFactory := NewIdentifierFactory() - conversionContext := NewStorageConversionContext(c.types) + conversionContext := NewStorageConversionContext(c.types, idFactory) currentType, ok := AsObjectType(c.currentObject.Type()) g.Expect(ok).To(BeTrue()) diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden index e758ccd5957..71879f7e4d4 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenOptionalObjectAndOptionalObject.golden @@ -4,7 +4,7 @@ package vCurrent import ( - "github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext" + "github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext" "github.com/pkg/errors" ) diff --git a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden index 8b0f51ca39b..d949d0863ca 100644 --- a/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden +++ b/hack/generator/pkg/astmodel/testdata/ConvertBetweenRequiredObjectAndOptionalObject.golden @@ -4,7 +4,7 @@ package vCurrent import ( - "github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext" + "github.com/Azure/azure-service-operator/hack/generated/_apis/Verification/vNext" "github.com/pkg/errors" ) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index f3a1a254e1b..0ceb8846c5b 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -160,7 +160,7 @@ func (f *StorageTypeFactory) injectConversions(name astmodel.TypeName) error { } // Create conversion functions - conversionContext := astmodel.NewStorageConversionContext(f.types) + conversionContext := astmodel.NewStorageConversionContext(f.types, f.idFactory) convertFrom, err := astmodel.NewStorageConversionFromFunction(def, nextDef, f.idFactory, conversionContext) if err != nil { From 7de2c995bea612617513d49b4330712b84eea4ea Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Mon, 24 May 2021 14:57:48 +1200 Subject: [PATCH 47/61] Ensure pointers are captured, not values --- .../pkg/codegen/storage/function_injector.go | 4 ++-- .../pkg/codegen/storage/property_converter.go | 14 +++++++------- .../pkg/codegen/storage/type_converter.go | 16 ++++++++-------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/function_injector.go b/hack/generator/pkg/codegen/storage/function_injector.go index f772c1ff17d..aae3b4ffa94 100644 --- a/hack/generator/pkg/codegen/storage/function_injector.go +++ b/hack/generator/pkg/codegen/storage/function_injector.go @@ -31,7 +31,7 @@ func (fi *FunctionInjector) Inject(def astmodel.TypeDefinition, fn astmodel.Func // injectFunctionIntoObject takes the function provided as a context and includes it on the // provided object type -func (_ FunctionInjector) injectFunctionIntoObject( +func (_ *FunctionInjector) injectFunctionIntoObject( _ *astmodel.TypeVisitor, ot *astmodel.ObjectType, ctx interface{}) (astmodel.Type, error) { fn := ctx.(astmodel.Function) return ot.WithFunction(fn), nil @@ -39,7 +39,7 @@ func (_ FunctionInjector) injectFunctionIntoObject( // injectFunctionIntoResource takes the function provided as a context and includes it on the // provided resource type -func (_ FunctionInjector) injectFunctionIntoResource( +func (_ *FunctionInjector) injectFunctionIntoResource( _ *astmodel.TypeVisitor, rt *astmodel.ResourceType, ctx interface{}) (astmodel.Type, error) { fn := ctx.(astmodel.Function) return rt.WithFunction(fn), nil diff --git a/hack/generator/pkg/codegen/storage/property_converter.go b/hack/generator/pkg/codegen/storage/property_converter.go index 949cb592111..dfd08661cd0 100644 --- a/hack/generator/pkg/codegen/storage/property_converter.go +++ b/hack/generator/pkg/codegen/storage/property_converter.go @@ -39,7 +39,7 @@ func NewPropertyConverter(types astmodel.Types) *PropertyConverter { } // ConvertProperty applies our conversion rules to a specific property -func (p PropertyConverter) ConvertProperty(property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { +func (p *PropertyConverter) ConvertProperty(property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { for _, conv := range p.propertyConversions { prop, err := conv(property) @@ -57,7 +57,7 @@ func (p PropertyConverter) ConvertProperty(property *astmodel.PropertyDefinition } // stripAllValidations removes all validations -func (p PropertyConverter) stripAllValidations( +func (p *PropertyConverter) stripAllValidations( this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { // strip all type validations from storage property types // act as if they do not exist @@ -65,7 +65,7 @@ func (p PropertyConverter) stripAllValidations( } // useBaseTypeForEnumerations replaces an enumeration with its underlying base type -func (p PropertyConverter) useBaseTypeForEnumerations( +func (p *PropertyConverter) useBaseTypeForEnumerations( tv *astmodel.TypeVisitor, et *astmodel.EnumType, ctx interface{}) (astmodel.Type, error) { return tv.Visit(et.BaseType(), ctx) } @@ -73,7 +73,7 @@ func (p PropertyConverter) useBaseTypeForEnumerations( // shortCircuitNamesOfSimpleTypes redirects TypeNames that reference resources or objects into our // storage namespace, and replaces TypeNames that point to simple types (enumerations or // primitives) with their underlying type. -func (p PropertyConverter) shortCircuitNamesOfSimpleTypes( +func (p *PropertyConverter) shortCircuitNamesOfSimpleTypes( tv *astmodel.TypeVisitor, tn astmodel.TypeName, ctx interface{}) (astmodel.Type, error) { actualType, err := p.types.FullyResolve(tn) @@ -99,7 +99,7 @@ func (p PropertyConverter) shortCircuitNamesOfSimpleTypes( return tv.Visit(actualType, ctx) } -func (_ PropertyConverter) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { +func (_ *PropertyConverter) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { // Map the type name into our storage namespace localRef, ok := name.PackageReference.AsLocalPackage() if !ok { @@ -118,7 +118,7 @@ type propertyConversion = func(property *astmodel.PropertyDefinition) (*astmodel // preserveKubernetesResourceStorageProperties preserves properties required by the // KubernetesResource interface as they're always required exactly as declared -func (p PropertyConverter) preserveKubernetesResourceStorageProperties( +func (p *PropertyConverter) preserveKubernetesResourceStorageProperties( prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { if astmodel.IsKubernetesResourceProperty(prop.PropertyName()) { @@ -130,7 +130,7 @@ func (p PropertyConverter) preserveKubernetesResourceStorageProperties( return nil, nil } -func (p PropertyConverter) defaultPropertyConversion( +func (p *PropertyConverter) defaultPropertyConversion( property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { propertyType, err := p.visitor.Visit(property.PropertyType(), nil) diff --git a/hack/generator/pkg/codegen/storage/type_converter.go b/hack/generator/pkg/codegen/storage/type_converter.go index e90e9a67058..1d10ebbd519 100644 --- a/hack/generator/pkg/codegen/storage/type_converter.go +++ b/hack/generator/pkg/codegen/storage/type_converter.go @@ -38,7 +38,7 @@ func NewTypeConverter(types astmodel.Types) *TypeConverter { } // ConvertDefinition applies our type conversion to a specific type definition -func (t TypeConverter) ConvertDefinition(def astmodel.TypeDefinition) (astmodel.TypeDefinition, error) { +func (t *TypeConverter) ConvertDefinition(def astmodel.TypeDefinition) (astmodel.TypeDefinition, error) { result, err := t.visitor.VisitDefinition(def, nil) if err != nil { return astmodel.TypeDefinition{}, errors.Wrapf(err, "converting %q for storage variant", def.Name()) @@ -55,7 +55,7 @@ func (t TypeConverter) ConvertDefinition(def astmodel.TypeDefinition) (astmodel. */ // convertResourceType creates a storage variation of a resource type -func (t TypeConverter) convertResourceType( +func (t *TypeConverter) convertResourceType( tv *astmodel.TypeVisitor, resource *astmodel.ResourceType, ctx interface{}) (astmodel.Type, error) { @@ -67,7 +67,7 @@ func (t TypeConverter) convertResourceType( } // convertObjectType creates a storage variation of an object type -func (t TypeConverter) convertObjectType( +func (t *TypeConverter) convertObjectType( _ *astmodel.TypeVisitor, object *astmodel.ObjectType, _ interface{}) (astmodel.Type, error) { var errs []error @@ -91,7 +91,7 @@ func (t TypeConverter) convertObjectType( } // redirectTypeNamesToStoragePackage modifies TypeNames to reference the current storage package -func (t TypeConverter) redirectTypeNamesToStoragePackage( +func (t *TypeConverter) redirectTypeNamesToStoragePackage( _ *astmodel.TypeVisitor, name astmodel.TypeName, _ interface{}) (astmodel.Type, error) { if result, ok := t.tryConvertToStorageNamespace(name); ok { return result, nil @@ -101,7 +101,7 @@ func (t TypeConverter) redirectTypeNamesToStoragePackage( } // stripAllValidations removes all validations -func (t TypeConverter) stripAllValidations( +func (t *TypeConverter) stripAllValidations( this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { // strip all type validations from storage types, // act as if they do not exist @@ -109,7 +109,7 @@ func (t TypeConverter) stripAllValidations( } // stripAllFlags removes all flags -func (t TypeConverter) stripAllFlags( +func (t *TypeConverter) stripAllFlags( tv *astmodel.TypeVisitor, flaggedType *astmodel.FlaggedType, ctx interface{}) (astmodel.Type, error) { @@ -121,7 +121,7 @@ func (t TypeConverter) stripAllFlags( return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) } -func (_ TypeConverter) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { +func (_ *TypeConverter) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { // Map the type name into our storage namespace localRef, ok := name.PackageReference.AsLocalPackage() if !ok { @@ -135,7 +135,7 @@ func (_ TypeConverter) tryConvertToStorageNamespace(name astmodel.TypeName) (ast // descriptionForStorageVariant creates a description for a storage variant, indicating which // original type it is based upon -func (_ TypeConverter) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { +func (_ *TypeConverter) descriptionForStorageVariant(definition astmodel.TypeDefinition) []string { pkg := definition.Name().PackageReference.PackageName() result := []string{ From e0b6a65c0d6cd892b07cdb2cd64a1f20b4184e54 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 26 May 2021 09:19:07 +1200 Subject: [PATCH 48/61] Add Process() to Types --- hack/generator/pkg/astmodel/types.go | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/hack/generator/pkg/astmodel/types.go b/hack/generator/pkg/astmodel/types.go index 51376989707..e38943fa6e4 100644 --- a/hack/generator/pkg/astmodel/types.go +++ b/hack/generator/pkg/astmodel/types.go @@ -7,7 +7,6 @@ package astmodel import ( "fmt" - "github.com/pkg/errors" ) @@ -204,3 +203,22 @@ func (types Types) ResolveResourceStatusDefinition( // preserve outer spec name return resourceStatusDef.WithName(statusName), nil } + +// Process applies a func to transform all members of this set of type definitions, returning a new set of type +// definitions containing the results of the transfomration, or possibly an error +// Only definitions returned by the func will be included in the results of the function. The func may return a nil +// TypeDefinition if it doesn't want to include anything in the output set. +func (types Types) Process(transformation func (definition TypeDefinition) (*TypeDefinition, error)) (Types, error) { + result := make(Types) + + for _, def := range types { + d, err := transformation(def) + if err != nil { + return nil, err + } else if d != nil { + result.Add(*d) + } + } + + return result, nil +} From ace581a7239a167cbcf895d0967a10bd6c389ba5 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 26 May 2021 09:19:29 +1200 Subject: [PATCH 49/61] Preserve ResourceReferences --- .../pkg/codegen/storage/property_converter.go | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/property_converter.go b/hack/generator/pkg/codegen/storage/property_converter.go index dfd08661cd0..920bf03858c 100644 --- a/hack/generator/pkg/codegen/storage/property_converter.go +++ b/hack/generator/pkg/codegen/storage/property_converter.go @@ -11,7 +11,7 @@ import ( "github.com/pkg/errors" ) -// PropertyConverter is used to convert the properties of object types as required for storage variants +// PropertyConverter is used to convert the properties of object inputTypes as required for storage variants type PropertyConverter struct { visitor astmodel.TypeVisitor // Visitor used to achieve the required modification types astmodel.Types // All the types for this group @@ -26,6 +26,7 @@ func NewPropertyConverter(types astmodel.Types) *PropertyConverter { result.propertyConversions = []propertyConversion{ result.preserveKubernetesResourceStorageProperties, + result.preserveResourceReferenceProperties, result.defaultPropertyConversion, } @@ -59,7 +60,7 @@ func (p *PropertyConverter) ConvertProperty(property *astmodel.PropertyDefinitio // stripAllValidations removes all validations func (p *PropertyConverter) stripAllValidations( this *astmodel.TypeVisitor, v *astmodel.ValidatedType, ctx interface{}) (astmodel.Type, error) { - // strip all type validations from storage property types + // strip all type validations from storage property inputTypes // act as if they do not exist return this.Visit(v.ElementType(), ctx) } @@ -130,6 +131,29 @@ func (p *PropertyConverter) preserveKubernetesResourceStorageProperties( return nil, nil } +// preserveResourceReferenceProperties preserves properties required by the +// KubernetesResource interface as they're always required exactly as declared +func (p *PropertyConverter) preserveResourceReferenceProperties( + prop *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { + + propertyType := prop.PropertyType() + if opt, ok := astmodel.AsOptionalType(propertyType); ok { + if opt.Element().Equals(astmodel.ResourceReferenceTypeName) { + // Keep these unchanged + return prop, nil + } + } + + if propertyType.Equals(astmodel.ResourceReferenceTypeName) { + // Keep these unchanged + return prop, nil + } + + // Not a resource reference property, defer to another conversion + return nil, nil +} + + func (p *PropertyConverter) defaultPropertyConversion( property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { From 366eb26c23a5d812dd6bbcd46460172dc73ee057 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 26 May 2021 11:28:41 +1200 Subject: [PATCH 50/61] Code gardening --- hack/generator/pkg/astbuilder/builder.go | 1 + hack/generator/pkg/astmodel/std_references.go | 2 +- .../astmodel/storage_conversion_function.go | 2 +- .../pkg/codegen/storage/function_injector.go | 3 ++- .../pkg/codegen/storage/hub_version_marker.go | 5 +++-- .../pkg/codegen/storage/property_converter.go | 19 ++++++++++++------- .../pkg/codegen/storage/type_converter.go | 9 ++++++--- 7 files changed, 26 insertions(+), 15 deletions(-) diff --git a/hack/generator/pkg/astbuilder/builder.go b/hack/generator/pkg/astbuilder/builder.go index bad167d77c8..80dad1bd9b8 100644 --- a/hack/generator/pkg/astbuilder/builder.go +++ b/hack/generator/pkg/astbuilder/builder.go @@ -291,6 +291,7 @@ func ReturnNoError() dst.Stmt { // // errors.Wrap(err, ) // +// (actual package name will be used, which will usually be 'errors') func WrappedErrorf(errorsPackage string, template string, args ...interface{}) dst.Expr { return CallQualifiedFunc( errorsPackage, diff --git a/hack/generator/pkg/astmodel/std_references.go b/hack/generator/pkg/astmodel/std_references.go index a1102f84c11..5fb5b11815e 100644 --- a/hack/generator/pkg/astmodel/std_references.go +++ b/hack/generator/pkg/astmodel/std_references.go @@ -13,7 +13,7 @@ var ( TestingReference = MakeExternalPackageReference("testing") // References to our Libraries - GenRuntimeReference PackageReference = MakeExternalPackageReference(genRuntimePathPrefix) + GenRuntimeReference = MakeExternalPackageReference(genRuntimePathPrefix) // References to other libraries ErrorsReference = MakeExternalPackageReference("github.com/pkg/errors") diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index 048347707c3..bbdf2683723 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -360,7 +360,7 @@ func (fn *StorageConversionFunction) createConversions(receiver TypeDefinition, return nil } -// AsPropertyContainer converts a type into a property container +// asPropertyContainer converts a type into a property container func (fn *StorageConversionFunction) asPropertyContainer(theType Type) (PropertyContainer, bool) { switch t := theType.(type) { case PropertyContainer: diff --git a/hack/generator/pkg/codegen/storage/function_injector.go b/hack/generator/pkg/codegen/storage/function_injector.go index aae3b4ffa94..8b238c715c1 100644 --- a/hack/generator/pkg/codegen/storage/function_injector.go +++ b/hack/generator/pkg/codegen/storage/function_injector.go @@ -9,7 +9,8 @@ import "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" // FunctionInjector is a utility for injecting function definitions into resources and objects type FunctionInjector struct { - visitor astmodel.TypeVisitor // Visitor used to achieve the required modification + // visitor is used to do the actual injection + visitor astmodel.TypeVisitor } // NewFunctionInjector creates a new function injector for modifying resources and objects diff --git a/hack/generator/pkg/codegen/storage/hub_version_marker.go b/hack/generator/pkg/codegen/storage/hub_version_marker.go index 58f3e41905d..b28e4e72243 100644 --- a/hack/generator/pkg/codegen/storage/hub_version_marker.go +++ b/hack/generator/pkg/codegen/storage/hub_version_marker.go @@ -9,7 +9,8 @@ import "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" // HubVersionMarker is a utility for marking resource types as "hub" versions type HubVersionMarker struct { - visitor astmodel.TypeVisitor // Visitor used to achieve the required modification + // visitor is used to do the actual marking + visitor astmodel.TypeVisitor } // NewHubVersionMarker returns a new hub version marker for flagging resource types @@ -23,7 +24,7 @@ func NewHubVersionMarker() *HubVersionMarker { return result } -// MarkAsStorageVersion mark the supplied type definition as the storage version +// MarkAsStorageVersion marks the supplied type definition as the storage version func (m *HubVersionMarker) MarkAsStorageVersion(def astmodel.TypeDefinition) (astmodel.TypeDefinition, error) { return m.visitor.VisitDefinition(def, nil) } diff --git a/hack/generator/pkg/codegen/storage/property_converter.go b/hack/generator/pkg/codegen/storage/property_converter.go index 920bf03858c..8cb1107ddc6 100644 --- a/hack/generator/pkg/codegen/storage/property_converter.go +++ b/hack/generator/pkg/codegen/storage/property_converter.go @@ -13,9 +13,12 @@ import ( // PropertyConverter is used to convert the properties of object inputTypes as required for storage variants type PropertyConverter struct { - visitor astmodel.TypeVisitor // Visitor used to achieve the required modification - types astmodel.Types // All the types for this group - propertyConversions []propertyConversion // Conversion rules to use for properties when creating storage variants + // visitor is used to apply the modification + visitor astmodel.TypeVisitor + // types contains all the types for this group + types astmodel.Types + // propertyConversions is an ordered list of all our conversion rules for creating storage variants + propertyConversions []propertyConversion } // NewPropertyConverter creates a new property converter for modifying object properties @@ -71,9 +74,12 @@ func (p *PropertyConverter) useBaseTypeForEnumerations( return tv.Visit(et.BaseType(), ctx) } -// shortCircuitNamesOfSimpleTypes redirects TypeNames that reference resources or objects into our -// storage namespace, and replaces TypeNames that point to simple types (enumerations or -// primitives) with their underlying type. +// shortCircuitNamesOfSimpleTypes redirects or replaces TypeNames +// o If a TypeName points into an API namespace, it is redirected into the appropriate storage namespace +// o If a TypeName references an enumeration, it is replaced with the underlying type of the enumeration as our +// storage types don't use enumerations, they use primitive types +// o If a TypeName references an alias for a primitive type (these are used to specify validations), it is replace +// with the primitive type func (p *PropertyConverter) shortCircuitNamesOfSimpleTypes( tv *astmodel.TypeVisitor, tn astmodel.TypeName, ctx interface{}) (astmodel.Type, error) { @@ -153,7 +159,6 @@ func (p *PropertyConverter) preserveResourceReferenceProperties( return nil, nil } - func (p *PropertyConverter) defaultPropertyConversion( property *astmodel.PropertyDefinition) (*astmodel.PropertyDefinition, error) { diff --git a/hack/generator/pkg/codegen/storage/type_converter.go b/hack/generator/pkg/codegen/storage/type_converter.go index 1d10ebbd519..9eb6609c9c4 100644 --- a/hack/generator/pkg/codegen/storage/type_converter.go +++ b/hack/generator/pkg/codegen/storage/type_converter.go @@ -14,9 +14,12 @@ import ( // TypeConverter is used to create a storage variant of an API type type TypeConverter struct { - visitor astmodel.TypeVisitor // Visitor used to achieve the required modification - types astmodel.Types // All the types for this group - propertyConverter *PropertyConverter // nested converter for properties + // visitor used to apply the modification + visitor astmodel.TypeVisitor + // types contains all the types for this group + types astmodel.Types + // propertyConverter is used to modify properties + propertyConverter *PropertyConverter } // NewTypeConverter creates a new instance of the utility type From 6d2ce02a43006f3aa1d66392a1ffd8263cc3e192 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 26 May 2021 11:29:22 +1200 Subject: [PATCH 51/61] Add support for ResourceReference properties --- .../astmodel/storage_conversion_factories.go | 52 ++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_factories.go b/hack/generator/pkg/astmodel/storage_conversion_factories.go index e5dd57720de..57e1a1526ed 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_factories.go +++ b/hack/generator/pkg/astmodel/storage_conversion_factories.go @@ -49,6 +49,7 @@ func init() { assignObjectTypeFromObjectType, // Known types assignKnownReferenceFromKnownReference, + assignResourceReferenceFromResourceReference, // Meta-conversions assignFromOptionalType, // Must go before assignToOptionalType so we generate the right zero values assignToOptionalType, @@ -124,6 +125,8 @@ func createTypeConversion( } } + // No conversion found, we need to generate a useful error message + var debugDescriptionOfDestination strings.Builder destinationEndpoint.Type().WriteDebugDescription(&debugDescriptionOfDestination, conversionContext.types) @@ -910,7 +913,7 @@ func assignKnownReferenceFromKnownReference( } // Require source to be a KnownResourceReference - if sourceName.Name() != "KnownResourceReference" { + if sourceName.Name() != "KnownResourceReference" && sourceName.PackageReference.Equals(GenRuntimeReference) { return nil } @@ -924,6 +927,53 @@ func assignKnownReferenceFromKnownReference( } } +// assignResourceReferenceFromResourceReference will generate a direct assignment if both types are genruntime.ResourceReference +// +// = .Copy() +// +func assignResourceReferenceFromResourceReference( + sourceEndpoint *StorageConversionEndpoint, + destinationEndpoint *StorageConversionEndpoint, + _ *StorageConversionContext) StorageTypeConversion { + + // Require source to be non-optional + if _, sourceIsOptional := AsOptionalType(sourceEndpoint.Type()); sourceIsOptional { + return nil + } + + // Require destination to be non-optional + if _, destinationIsOptional := AsOptionalType(destinationEndpoint.Type()); destinationIsOptional { + return nil + } + + // Require source to be a named type + sourceName, sourceIsName := AsTypeName(sourceEndpoint.Type()) + if !sourceIsName { + return nil + } + + // Require destination to be a named type + destinationName, destinationIsName := AsTypeName(destinationEndpoint.Type()) + if !destinationIsName { + return nil + } + + // Require source to be a ResourceReference + if sourceName.Name() != "ResourceReference" && sourceName.PackageReference.Equals(GenRuntimeReference) { + return nil + } + + // Require destination to be a ResourceReference + if destinationName.Name() != "ResourceReference" && destinationName.PackageReference.Equals(GenRuntimeReference){ + return nil + } + + return func(reader dst.Expr, writer func(dst.Expr) []dst.Stmt, generationContext *CodeGenerationContext) []dst.Stmt { + return writer(astbuilder.CallExpr(reader, "Copy")) + } +} + + func createTypeDeclaration(name TypeName, generationContext *CodeGenerationContext) dst.Expr { if name.PackageReference.Equals(generationContext.CurrentPackage()) { return dst.NewIdent(name.Name()) From f1b5efe64fe38eaceb4f585029a36b29630e02ce Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 26 May 2021 11:29:52 +1200 Subject: [PATCH 52/61] Simplify error handling when creating conversions --- .../astmodel/storage_conversion_function.go | 28 ++----------------- 1 file changed, 3 insertions(+), 25 deletions(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index bbdf2683723..7e3d4d6de34 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -14,7 +14,6 @@ import ( "github.com/Azure/azure-service-operator/hack/generator/pkg/astbuilder" "github.com/dave/dst" "github.com/pkg/errors" - "k8s.io/klog/v2" ) // StorageConversionFunction represents a function that performs conversions for storage versions @@ -308,9 +307,6 @@ func (fn *StorageConversionFunction) createConversions(receiver TypeDefinition, receiverProperties := fn.createPropertyMap(receiverContainer) otherProperties := fn.createPropertyMap(otherContainer) - var properties []string - first := true - // Flag receiver name as used fn.knownLocals.Add(receiver.Name().Name()) @@ -330,33 +326,15 @@ func (fn *StorageConversionFunction) createConversions(receiver TypeDefinition, } if err != nil { - // An error was returned; this can happen even if a conversion was created as well. - if first { - klog.V(2).Infof("Generating conversion function %s() for %s", fn.name, receiver.name) - first = false - } - - // err will include the details of which property, don't need to log here - klog.V(2).Infof("Error: %s", err) - properties = append(properties, string(receiverProperty.propertyName)) - continue - } - - if conv != nil { + // An error was returned, we abort creating conversions for this object + return errors.Wrapf(err, "creating conversion for property %q of %q", receiverProperty.PropertyName(), receiver.Name()) + } else if conv != nil { // A conversion was created, keep it for later fn.conversions[string(receiverProperty.propertyName)] = conv } } } - if len(properties) > 0 { - label := "properties" - if len(properties) == 1 { - label = "property" - } - return errors.Errorf("Errors with %d %s: %s", len(properties), label, strings.Join(properties, "; ")) - } - return nil } From 3bcd98b9b8dd42753b92d9364fba31e704021284 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 26 May 2021 11:32:28 +1200 Subject: [PATCH 53/61] Refactor StorageTypeFactory --- .../codegen/storage/storage_type_factory.go | 186 ++++++++---------- 1 file changed, 84 insertions(+), 102 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 0ceb8846c5b..2b4b58d389d 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -11,74 +11,88 @@ import ( "k8s.io/klog/v2" ) -// StorageTypeFactory is used to create storage types for a specific service +// StorageTypeFactory is used to create storage inputTypes for a specific service type StorageTypeFactory struct { - service string // Name of the group we're handling (used mostly for logging) - types astmodel.Types // All the types for this group - pendingStorageConversion astmodel.TypeNameQueue // Queue of types that need storage variants created for them - pendingConversionInjection astmodel.TypeNameQueue // Queue of types that need conversion functions injected - pendingMarkAsHubVersion astmodel.TypeNameQueue // Queue of types that need to be flagged as the hub storage version - idFactory astmodel.IdentifierFactory // Factory for creating identifiers - typeConverter *TypeConverter // a utility type type visitor used to create storage variants - functionInjector *FunctionInjector // a utility used to inject functions into definitions - resourceHubMarker *HubVersionMarker // a utility used to mark resources as Storage Versions - conversionMap map[astmodel.PackageReference]astmodel.PackageReference // Map of conversion links for creating our conversion graph + service string // Name of the group we're handling (used mostly for logging) + inputTypes astmodel.Types // All the types for this group + outputTypes astmodel.Types // All the types created/modified by this factory + idFactory astmodel.IdentifierFactory // Factory for creating identifiers + typeConverter *TypeConverter // a utility type type visitor used to create storage variants + functionInjector *FunctionInjector // a utility used to inject functions into definitions + resourceHubMarker *HubVersionMarker // a utility used to mark resources as Storage Versions + conversionMap map[astmodel.PackageReference]astmodel.PackageReference // Map of conversion links for creating our conversion graph } // NewStorageTypeFactory creates a new instance of StorageTypeFactory ready for use func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) *StorageTypeFactory { + + types := make(astmodel.Types) + result := &StorageTypeFactory{ service: service, - types: make(astmodel.Types), - pendingStorageConversion: astmodel.MakeTypeNameQueue(), - pendingConversionInjection: astmodel.MakeTypeNameQueue(), - pendingMarkAsHubVersion: astmodel.MakeTypeNameQueue(), + inputTypes: types, idFactory: idFactory, conversionMap: make(map[astmodel.PackageReference]astmodel.PackageReference), functionInjector: NewFunctionInjector(), resourceHubMarker: NewHubVersionMarker(), + typeConverter: NewTypeConverter(types), } - result.typeConverter = NewTypeConverter(result.types) - return result } // Add the supplied type definition to this factory func (f *StorageTypeFactory) Add(def astmodel.TypeDefinition) { - f.types.Add(def) - - // Add to our queue of types requiring storage variants - f.pendingStorageConversion.Enqueue(def.Name()) + f.inputTypes.Add(def) } -// Types returns types contained by the factory, including all new storage variants and modified -// api types. If any errors occur during processing, they're returned here. +// Types returns inputTypes contained by the factory, including all new storage variants and modified +// api inputTypes. If any errors occur during processing, they're returned here. func (f *StorageTypeFactory) Types() (astmodel.Types, error) { - err := f.process() - if err != nil { - return nil, err + if len(f.outputTypes) == 0 { + err := f.process() + if err != nil { + return nil, err + } } - return f.types, nil + return f.outputTypes, nil } func (f *StorageTypeFactory) process() error { - err := f.pendingStorageConversion.Process(f.createStorageVariant) + + f.outputTypes = make(astmodel.Types) + + // Create storage variants and stash them in outputTypes so injectConversions can find them + storageVariants, err := f.inputTypes.Process(f.createStorageVariant) if err != nil { return err } - err = f.pendingConversionInjection.Process(f.injectConversions) + f.outputTypes.AddTypes(storageVariants) + + // Inject conversions into our original types and stash them in outputTypes + inputTypesWithConversions, err := f.inputTypes.Process(f.injectConversions) if err != nil { return err } - err = f.pendingMarkAsHubVersion.Process(f.markAsHubVersion) + f.outputTypes.AddTypes(inputTypesWithConversions) + + // Inject conversions into our storage variants and replace the definitions in outputTypes + storageVariantsWithConversions, err := storageVariants.Process(f.injectConversions) if err != nil { return err } + for _, d := range storageVariantsWithConversions { + f.outputTypes[d.Name()] = d + } + + + // Add anything missing into outputTypes + f.outputTypes.AddTypes(f.inputTypes.Except(f.outputTypes)) + return nil } @@ -86,60 +100,43 @@ func (f *StorageTypeFactory) process() error { // related package. // def is the api definition on which to base the storage variant // visitor is a type visitor that will do the creation -func (f *StorageTypeFactory) createStorageVariant(name astmodel.TypeName) error { - - // Only need to create storage variants of resources and objects - underlyingType, err := f.types.FullyResolve(name) - if err != nil { - return errors.Wrapf(err, - "expected to find underlying type for %q", - name) - } - - _, isObject := astmodel.AsObjectType(underlyingType) - _, isResource := astmodel.AsResourceType(underlyingType) +func (f *StorageTypeFactory) createStorageVariant(definition astmodel.TypeDefinition) (*astmodel.TypeDefinition, error) { + name := definition.Name() + _, isObject := astmodel.AsObjectType(definition.Type()) + _, isResource := astmodel.AsResourceType(definition.Type()) if !isObject && !isResource { // just skip it klog.V(4).Infof("Skipping %s as no storage variant needed", name) - return nil + return nil, nil } klog.V(3).Infof("Creating storage variant of %s", name) - def, ok := f.types[name] - if !ok { - return errors.Errorf("failed to find definition for %q", name) - } - - storageDef, err := f.typeConverter.ConvertDefinition(def) + storageDef, err := f.typeConverter.ConvertDefinition(definition) if err != nil { - return errors.Wrapf(err, "creating storage variant for %q", name) + return nil, errors.Wrapf(err, "creating storage variant for %q", name) } - f.types.Add(storageDef) - // Add API-Package -> Storage-Package link into the conversion map f.conversionMap[name.PackageReference] = storageDef.Name().PackageReference - // Queue for injection of conversion functions - f.pendingConversionInjection.Enqueue(name) - - //TODO: Queue storage type for injection of conversion too - - return nil + return &storageDef, nil } // injectConversions modifies the named type by injecting the required conversion methods using // the conversionMap we've previously established -func (f *StorageTypeFactory) injectConversions(name astmodel.TypeName) error { - klog.V(3).Infof("Injecting conversion functions into %s", name) - - // Find the definition to modify - def, ok := f.types[name] - if !ok { - return errors.Errorf("failed to find definition for %q", name) +func (f *StorageTypeFactory) injectConversions(definition astmodel.TypeDefinition) (*astmodel.TypeDefinition, error) { + name := definition.Name() + _, isObject := astmodel.AsObjectType(definition.Type()) + _, isResource := astmodel.AsResourceType(definition.Type()) + if !isObject && !isResource { + // just skip it + klog.V(4).Infof("Skipping %s as no conversion functions needed", name) + return nil, nil } + klog.V(3).Infof("Injecting conversion functions into %s", name) + // Find the definition we want to convert to/from nextPackage, ok := f.conversionMap[name.PackageReference] if !ok { @@ -147,62 +144,47 @@ func (f *StorageTypeFactory) injectConversions(name astmodel.TypeName) error { // (this is expected if we have the hub storage package) // Flag the type as needing to be flagged as the storage version //TODO: Restore this - currently disabled until we get all the conversion functions injected - //!! f.pendingMarkAsHubVersion.Enqueue(name) - return nil + //hubDefintion, err := f.resourceHubMarker.markResourceAsStorageVersion(definition) + //if err != nil { + // return nil, errors.Wrapf(err, "marking %q as hub version", name) + //} + //return &hubDefintion, nil + return nil, nil } nextName := astmodel.MakeTypeName(nextPackage, name.Name()) - nextDef, ok := f.types[nextName] + nextDef, ok := f.outputTypes[nextName] if !ok { // No next type so nothing to do - // (this is expected if the type is discontinued) - return nil + // (this is expected if the type is discontinued or we're looking at the hub type) + return nil, nil } // Create conversion functions - conversionContext := astmodel.NewStorageConversionContext(f.types, f.idFactory) + knownTypes := make(astmodel.Types) + knownTypes.AddTypes(f.inputTypes) + knownTypes.AddTypes(f.outputTypes) + conversionContext := astmodel.NewStorageConversionContext(knownTypes, f.idFactory) - convertFrom, err := astmodel.NewStorageConversionFromFunction(def, nextDef, f.idFactory, conversionContext) + convertFromFn, err := astmodel.NewStorageConversionFromFunction(definition, nextDef, f.idFactory, conversionContext) if err != nil { - return errors.Wrapf(err, "creating ConvertFrom() function for %q", name) + return nil, errors.Wrapf(err, "creating ConvertFrom() function for %q", name) } - convertTo, err := astmodel.NewStorageConversionToFunction(def, nextDef, f.idFactory, conversionContext) + convertToFn, err := astmodel.NewStorageConversionToFunction(definition, nextDef, f.idFactory, conversionContext) if err != nil { - return errors.Wrapf(err, "creating ConvertTo() function for %q", name) + return nil, errors.Wrapf(err, "creating ConvertTo() function for %q", name) } - def, err = f.functionInjector.Inject(def, convertFrom) + updatedDefinition, err := f.functionInjector.Inject(definition, convertFromFn) if err != nil { - return errors.Wrapf(err, "failed to inject ConvertFrom function into %q", name) + return nil, errors.Wrapf(err, "failed to inject ConvertFrom function into %q", name) } - def, err = f.functionInjector.Inject(def, convertTo) + updatedDefinition, err = f.functionInjector.Inject(updatedDefinition, convertToFn) if err != nil { - return errors.Wrapf(err, "failed to inject ConvertFrom function into %q", name) + return nil, errors.Wrapf(err, "failed to inject ConvertFrom function into %q", name) } - // Update our map - f.types[name] = def - - return nil -} - -func (f *StorageTypeFactory) markAsHubVersion(name astmodel.TypeName) error { - // Find the definition to modify - def, ok := f.types[name] - if !ok { - return errors.Errorf("failed to find definition for %q", name) - } - - // Mark the resource as the hub storage version - updated, err := f.resourceHubMarker.MarkAsStorageVersion(def) - if err != nil { - return errors.Wrapf(err, "marking %q as hub storage version", name) - } - - // Update our map - f.types[name] = updated - - return nil + return &updatedDefinition, nil } From 5a55825992b92f7e027f3bde967775ddc54bdfa3 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 26 May 2021 11:48:29 +1200 Subject: [PATCH 54/61] Add warning --- hack/generator/pkg/codegen/pipeline_create_storage_types.go | 1 + 1 file changed, 1 insertion(+) diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index 548d4c907d7..f1121b78bdf 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -30,6 +30,7 @@ func createStorageTypes(idFactory astmodel.IdentifierFactory) PipelineStage { if !ok { // Skip definitions from non-local packages // (should never happen) + klog.Warningf("Unexpectedly found non-local package reference %q", name.PackageReference) continue } From 53ed56cd7e225ebc88b986ed0adce42f7dfd83e4 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 26 May 2021 11:48:39 +1200 Subject: [PATCH 55/61] Simplify handling of aggregate errors --- hack/generator/pkg/codegen/pipeline_create_storage_types.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index f1121b78bdf..b7fc3909977 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -62,8 +62,8 @@ func createStorageTypes(idFactory astmodel.IdentifierFactory) PipelineStage { result.AddTypes(t) } - if len(errs) > 0 { - err := kerrors.NewAggregate(errs) + err := kerrors.NewAggregate(errs) + if err != nil { return nil, err } From 89009cd62fc7b02b994a2a30a6ce7aad828747fc Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 26 May 2021 11:51:23 +1200 Subject: [PATCH 56/61] Rename member to group --- .../codegen/storage/storage_type_factory.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 2b4b58d389d..6fcf117f468 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -13,7 +13,7 @@ import ( // StorageTypeFactory is used to create storage inputTypes for a specific service type StorageTypeFactory struct { - service string // Name of the group we're handling (used mostly for logging) + group string // Name of the group we're handling (used mostly for logging) inputTypes astmodel.Types // All the types for this group outputTypes astmodel.Types // All the types created/modified by this factory idFactory astmodel.IdentifierFactory // Factory for creating identifiers @@ -24,18 +24,18 @@ type StorageTypeFactory struct { } // NewStorageTypeFactory creates a new instance of StorageTypeFactory ready for use -func NewStorageTypeFactory(service string, idFactory astmodel.IdentifierFactory) *StorageTypeFactory { +func NewStorageTypeFactory(group string, idFactory astmodel.IdentifierFactory) *StorageTypeFactory { types := make(astmodel.Types) result := &StorageTypeFactory{ - service: service, - inputTypes: types, - idFactory: idFactory, - conversionMap: make(map[astmodel.PackageReference]astmodel.PackageReference), - functionInjector: NewFunctionInjector(), - resourceHubMarker: NewHubVersionMarker(), - typeConverter: NewTypeConverter(types), + group: group, + inputTypes: types, + idFactory: idFactory, + conversionMap: make(map[astmodel.PackageReference]astmodel.PackageReference), + functionInjector: NewFunctionInjector(), + resourceHubMarker: NewHubVersionMarker(), + typeConverter: NewTypeConverter(types), } return result @@ -89,7 +89,6 @@ func (f *StorageTypeFactory) process() error { f.outputTypes[d.Name()] = d } - // Add anything missing into outputTypes f.outputTypes.AddTypes(f.inputTypes.Except(f.outputTypes)) From 52c3aaf6db47a1669529f998a8266a1dc2367809 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 26 May 2021 12:00:31 +1200 Subject: [PATCH 57/61] Include property type in error --- hack/generator/pkg/codegen/storage/property_converter.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/hack/generator/pkg/codegen/storage/property_converter.go b/hack/generator/pkg/codegen/storage/property_converter.go index 8cb1107ddc6..f164142d7d5 100644 --- a/hack/generator/pkg/codegen/storage/property_converter.go +++ b/hack/generator/pkg/codegen/storage/property_converter.go @@ -9,6 +9,7 @@ import ( "fmt" "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" "github.com/pkg/errors" + "strings" ) // PropertyConverter is used to convert the properties of object inputTypes as required for storage variants @@ -57,7 +58,13 @@ func (p *PropertyConverter) ConvertProperty(property *astmodel.PropertyDefinitio } } - return nil, fmt.Errorf("failed to find a conversion for property %v", property.PropertyName()) + // No conversion found + + var typeDescription strings.Builder + property.PropertyType().WriteDebugDescription(&typeDescription, p.types) + + return nil, fmt.Errorf( + "failed to find a conversion for property %v (%v)", property.PropertyName(), typeDescription.String()) } // stripAllValidations removes all validations From 1cec0733a57720d293cb19fd848705da765ecf92 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Wed, 26 May 2021 14:15:41 +1200 Subject: [PATCH 58/61] Go fmt --- hack/generator/pkg/astmodel/std_references.go | 2 +- hack/generator/pkg/astmodel/storage_conversion_factories.go | 3 +-- hack/generator/pkg/astmodel/types.go | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/hack/generator/pkg/astmodel/std_references.go b/hack/generator/pkg/astmodel/std_references.go index 5fb5b11815e..704a1e70666 100644 --- a/hack/generator/pkg/astmodel/std_references.go +++ b/hack/generator/pkg/astmodel/std_references.go @@ -13,7 +13,7 @@ var ( TestingReference = MakeExternalPackageReference("testing") // References to our Libraries - GenRuntimeReference = MakeExternalPackageReference(genRuntimePathPrefix) + GenRuntimeReference = MakeExternalPackageReference(genRuntimePathPrefix) // References to other libraries ErrorsReference = MakeExternalPackageReference("github.com/pkg/errors") diff --git a/hack/generator/pkg/astmodel/storage_conversion_factories.go b/hack/generator/pkg/astmodel/storage_conversion_factories.go index 57e1a1526ed..aa20f156579 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_factories.go +++ b/hack/generator/pkg/astmodel/storage_conversion_factories.go @@ -964,7 +964,7 @@ func assignResourceReferenceFromResourceReference( } // Require destination to be a ResourceReference - if destinationName.Name() != "ResourceReference" && destinationName.PackageReference.Equals(GenRuntimeReference){ + if destinationName.Name() != "ResourceReference" && destinationName.PackageReference.Equals(GenRuntimeReference) { return nil } @@ -973,7 +973,6 @@ func assignResourceReferenceFromResourceReference( } } - func createTypeDeclaration(name TypeName, generationContext *CodeGenerationContext) dst.Expr { if name.PackageReference.Equals(generationContext.CurrentPackage()) { return dst.NewIdent(name.Name()) diff --git a/hack/generator/pkg/astmodel/types.go b/hack/generator/pkg/astmodel/types.go index e38943fa6e4..330f100f8bf 100644 --- a/hack/generator/pkg/astmodel/types.go +++ b/hack/generator/pkg/astmodel/types.go @@ -208,7 +208,7 @@ func (types Types) ResolveResourceStatusDefinition( // definitions containing the results of the transfomration, or possibly an error // Only definitions returned by the func will be included in the results of the function. The func may return a nil // TypeDefinition if it doesn't want to include anything in the output set. -func (types Types) Process(transformation func (definition TypeDefinition) (*TypeDefinition, error)) (Types, error) { +func (types Types) Process(transformation func(definition TypeDefinition) (*TypeDefinition, error)) (Types, error) { result := make(Types) for _, def := range types { From aa83be6d5f1d1cf7c4e8c358df43825d053252a0 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Thu, 27 May 2021 07:50:10 +1200 Subject: [PATCH 59/61] Apply suggestions from code review Co-authored-by: Matthew Christopher --- hack/generator/pkg/astmodel/storage_conversion_factories.go | 2 +- hack/generator/pkg/codegen/pipeline_create_storage_types.go | 2 +- hack/generator/pkg/codegen/storage/storage_type_factory.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/hack/generator/pkg/astmodel/storage_conversion_factories.go b/hack/generator/pkg/astmodel/storage_conversion_factories.go index aa20f156579..81ba97c8090 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_factories.go +++ b/hack/generator/pkg/astmodel/storage_conversion_factories.go @@ -831,7 +831,7 @@ func assignObjectTypeFromObjectType( return func(reader dst.Expr, writer func(dst.Expr) []dst.Stmt, generationContext *CodeGenerationContext) []dst.Stmt { // We have to do this at render time in order to ensure the first conversion generated - // declares 'err`, not a later one + // declares 'err', not a later one tok := token.ASSIGN if conversionContext.TryCreateLocal("err") { tok = token.DEFINE diff --git a/hack/generator/pkg/codegen/pipeline_create_storage_types.go b/hack/generator/pkg/codegen/pipeline_create_storage_types.go index b7fc3909977..2ebb33b24fc 100644 --- a/hack/generator/pkg/codegen/pipeline_create_storage_types.go +++ b/hack/generator/pkg/codegen/pipeline_create_storage_types.go @@ -30,7 +30,7 @@ func createStorageTypes(idFactory astmodel.IdentifierFactory) PipelineStage { if !ok { // Skip definitions from non-local packages // (should never happen) - klog.Warningf("Unexpectedly found non-local package reference %q", name.PackageReference) + klog.Warningf("Skipping storage type generation for unexpected non-local package reference %q", name.PackageReference) continue } diff --git a/hack/generator/pkg/codegen/storage/storage_type_factory.go b/hack/generator/pkg/codegen/storage/storage_type_factory.go index 6fcf117f468..903b22e0f2a 100644 --- a/hack/generator/pkg/codegen/storage/storage_type_factory.go +++ b/hack/generator/pkg/codegen/storage/storage_type_factory.go @@ -11,7 +11,7 @@ import ( "k8s.io/klog/v2" ) -// StorageTypeFactory is used to create storage inputTypes for a specific service +// StorageTypeFactory is used to create storage inputTypes for a specific api group type StorageTypeFactory struct { group string // Name of the group we're handling (used mostly for logging) inputTypes astmodel.Types // All the types for this group From 21df630896c05caf1fa3bae7c4ec06462334c432 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Thu, 27 May 2021 07:51:47 +1200 Subject: [PATCH 60/61] Fix terminology, using package instead of namespace --- .../pkg/codegen/storage/property_converter.go | 10 +++++----- hack/generator/pkg/codegen/storage/type_converter.go | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/hack/generator/pkg/codegen/storage/property_converter.go b/hack/generator/pkg/codegen/storage/property_converter.go index f164142d7d5..8a48c09c2c0 100644 --- a/hack/generator/pkg/codegen/storage/property_converter.go +++ b/hack/generator/pkg/codegen/storage/property_converter.go @@ -82,7 +82,7 @@ func (p *PropertyConverter) useBaseTypeForEnumerations( } // shortCircuitNamesOfSimpleTypes redirects or replaces TypeNames -// o If a TypeName points into an API namespace, it is redirected into the appropriate storage namespace +// o If a TypeName points into an API package, it is redirected into the appropriate storage package // o If a TypeName references an enumeration, it is replaced with the underlying type of the enumeration as our // storage types don't use enumerations, they use primitive types // o If a TypeName references an alias for a primitive type (these are used to specify validations), it is replace @@ -100,8 +100,8 @@ func (p *PropertyConverter) shortCircuitNamesOfSimpleTypes( _, isResource := astmodel.AsResourceType(actualType) if isObject || isResource { - // We have an object or a resource, redirect to our storage namespace if we can - if storageName, ok := p.tryConvertToStorageNamespace(tn); ok { + // We have an object or a resource, redirect to our storage package if we can + if storageName, ok := p.tryConvertToStoragePackage(tn); ok { return storageName, nil } @@ -113,8 +113,8 @@ func (p *PropertyConverter) shortCircuitNamesOfSimpleTypes( return tv.Visit(actualType, ctx) } -func (_ *PropertyConverter) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { - // Map the type name into our storage namespace +func (_ *PropertyConverter) tryConvertToStoragePackage(name astmodel.TypeName) (astmodel.TypeName, bool) { + // Map the type name into our storage package localRef, ok := name.PackageReference.AsLocalPackage() if !ok { return astmodel.TypeName{}, false diff --git a/hack/generator/pkg/codegen/storage/type_converter.go b/hack/generator/pkg/codegen/storage/type_converter.go index 9eb6609c9c4..b135da64b34 100644 --- a/hack/generator/pkg/codegen/storage/type_converter.go +++ b/hack/generator/pkg/codegen/storage/type_converter.go @@ -96,7 +96,7 @@ func (t *TypeConverter) convertObjectType( // redirectTypeNamesToStoragePackage modifies TypeNames to reference the current storage package func (t *TypeConverter) redirectTypeNamesToStoragePackage( _ *astmodel.TypeVisitor, name astmodel.TypeName, _ interface{}) (astmodel.Type, error) { - if result, ok := t.tryConvertToStorageNamespace(name); ok { + if result, ok := t.tryConvertToStoragePackage(name); ok { return result, nil } @@ -124,8 +124,8 @@ func (t *TypeConverter) stripAllFlags( return astmodel.IdentityVisitOfFlaggedType(tv, flaggedType, ctx) } -func (_ *TypeConverter) tryConvertToStorageNamespace(name astmodel.TypeName) (astmodel.TypeName, bool) { - // Map the type name into our storage namespace +func (_ *TypeConverter) tryConvertToStoragePackage(name astmodel.TypeName) (astmodel.TypeName, bool) { + // Map the type name into our storage package localRef, ok := name.PackageReference.AsLocalPackage() if !ok { return astmodel.TypeName{}, false From e28774c43e27ff80290ffd95ee89b9cf1c960637 Mon Sep 17 00:00:00 2001 From: Bevan Arps Date: Thu, 27 May 2021 11:35:47 +1200 Subject: [PATCH 61/61] Add GitHub errors reference --- hack/generator/pkg/astmodel/std_references.go | 1 + hack/generator/pkg/astmodel/storage_conversion_factories.go | 2 +- hack/generator/pkg/astmodel/storage_conversion_function.go | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/hack/generator/pkg/astmodel/std_references.go b/hack/generator/pkg/astmodel/std_references.go index d309bbabb67..0bc5791f304 100644 --- a/hack/generator/pkg/astmodel/std_references.go +++ b/hack/generator/pkg/astmodel/std_references.go @@ -23,6 +23,7 @@ var ( APIMachineryRuntimeReference = MakeExternalPackageReference("k8s.io/apimachinery/pkg/runtime") ClientGoSchemeReference = MakeExternalPackageReference("k8s.io/client-go/kubernetes/scheme") ControllerRuntimeAdmission = MakeExternalPackageReference("sigs.k8s.io/controller-runtime/pkg/webhook/admission") + GitHubErrorsReference = MakeExternalPackageReference("github.com/pkg/errors") // References to libraries used for testing CmpReference = MakeExternalPackageReference("github.com/google/go-cmp/cmp") diff --git a/hack/generator/pkg/astmodel/storage_conversion_factories.go b/hack/generator/pkg/astmodel/storage_conversion_factories.go index 81ba97c8090..d9fdd6350fa 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_factories.go +++ b/hack/generator/pkg/astmodel/storage_conversion_factories.go @@ -840,7 +840,7 @@ func assignObjectTypeFromObjectType( localId := dst.NewIdent(copyVar) errLocal := dst.NewIdent("err") - errorsPackageName := generationContext.MustGetImportedPackageName(ErrorsReference) + errorsPackageName := generationContext.MustGetImportedPackageName(GitHubErrorsReference) declaration := astbuilder.LocalVariableDeclaration(copyVar, createTypeDeclaration(destinationName, generationContext), "") diff --git a/hack/generator/pkg/astmodel/storage_conversion_function.go b/hack/generator/pkg/astmodel/storage_conversion_function.go index 7e3d4d6de34..eec9529d46c 100644 --- a/hack/generator/pkg/astmodel/storage_conversion_function.go +++ b/hack/generator/pkg/astmodel/storage_conversion_function.go @@ -118,7 +118,7 @@ func (fn *StorageConversionFunction) Name() string { // RequiredPackageReferences returns the set of package references required by this function func (fn *StorageConversionFunction) RequiredPackageReferences() *PackageReferenceSet { result := NewPackageReferenceSet( - ErrorsReference, + GitHubErrorsReference, fn.otherDefinition.Name().PackageReference) return result