From 5df37e78a28e42dabd0d0a99055ba50c566ca4a5 Mon Sep 17 00:00:00 2001 From: George Pollard Date: Fri, 9 Jul 2021 11:35:35 +1200 Subject: [PATCH] Improve flattening (#1631) 1. Flattening needed to visit arrays, maps, etc to match up inner types. 2. Added a panic if we have missed flattening any properties marked `flatten: true`; improve reporting of panics during code generation process to include package/typename. 3. If types are not modified then keep using the original type. This improves the typenames a bit (#1591). 4. When matching up spec/status types to augment specs with status, ignore any optionality when matching up types. This makes things work properly with the handcrafted schemas where often the `required` attribute has been forgotten. --- .../controllers/crd_cosmosdb_test.go | 2 +- hack/generated/controllers/crd_disk_test.go | 2 +- .../crd_networking_loadbalancer_test.go | 37 ++-- .../crd_networking_publicip_test.go | 6 +- .../controllers/crd_storageaccount_test.go | 5 +- hack/generated/controllers/crd_vmss_test.go | 98 +++++---- .../deployment_reconciler_negative_test.go | 13 +- hack/generated/controllers/edge_case_test.go | 17 +- hack/generator/azure-arm.yaml | 2 +- hack/generator/pkg/astmodel/allof_type.go | 8 +- hack/generator/pkg/astmodel/enum_type.go | 8 +- hack/generator/pkg/astmodel/errored_type.go | 32 ++- hack/generator/pkg/astmodel/flagged_type.go | 4 + hack/generator/pkg/astmodel/map_type.go | 2 +- hack/generator/pkg/astmodel/object_type.go | 5 +- hack/generator/pkg/astmodel/oneof_type.go | 8 +- hack/generator/pkg/astmodel/primitive_type.go | 7 +- .../pkg/astmodel/property_definition.go | 29 ++- .../pkg/astmodel/property_definition_test.go | 22 +- hack/generator/pkg/astmodel/resource_type.go | 5 - .../generator/pkg/astmodel/type_definition.go | 10 +- hack/generator/pkg/astmodel/type_name.go | 5 +- hack/generator/pkg/astmodel/validated_type.go | 4 + hack/generator/pkg/codegen/code_generator.go | 3 +- .../pipeline/add_cross_resource_references.go | 8 +- .../convert_allof_and_oneof_to_objects.go | 7 +- .../pkg/codegen/pipeline/create_arm_types.go | 6 +- .../pipeline/determine_resource_ownership.go | 8 +- .../codegen/pipeline/flatten_properties.go | 29 ++- .../pipeline/flatten_properties_test.go | 11 +- .../pkg/codegen/pipeline/status_augment.go | 188 +++++++++++++++--- 31 files changed, 396 insertions(+), 195 deletions(-) diff --git a/hack/generated/controllers/crd_cosmosdb_test.go b/hack/generated/controllers/crd_cosmosdb_test.go index da28d606cc6..4b7282dac00 100644 --- a/hack/generated/controllers/crd_cosmosdb_test.go +++ b/hack/generated/controllers/crd_cosmosdb_test.go @@ -32,7 +32,7 @@ func Test_CosmosDB_CRUD(t *testing.T) { Location: &tc.AzureRegion, Owner: testcommon.AsOwner(rg.ObjectMeta), Kind: &kind, - DatabaseAccountOfferType: documentdb.DatabaseAccountsSpecPropertiesDatabaseAccountOfferTypeStandard, + DatabaseAccountOfferType: documentdb.DatabaseAccountCreateUpdatePropertiesDatabaseAccountOfferTypeStandard, Locations: []documentdb.Location{ { LocationName: &tc.AzureRegion, diff --git a/hack/generated/controllers/crd_disk_test.go b/hack/generated/controllers/crd_disk_test.go index e57caed78e4..83e26d8dec9 100644 --- a/hack/generated/controllers/crd_disk_test.go +++ b/hack/generated/controllers/crd_disk_test.go @@ -49,7 +49,7 @@ func Test_Disk_CRUD(t *testing.T) { // Perform a simple patch. patcher := tc.NewResourcePatcher(disk) - networkAccessPolicy := compute.DisksSpecPropertiesNetworkAccessPolicyDenyAll + networkAccessPolicy := compute.DiskPropertiesNetworkAccessPolicyDenyAll disk.Spec.NetworkAccessPolicy = &networkAccessPolicy patcher.Patch(disk) diff --git a/hack/generated/controllers/crd_networking_loadbalancer_test.go b/hack/generated/controllers/crd_networking_loadbalancer_test.go index 6749c91c642..cde79bdc768 100644 --- a/hack/generated/controllers/crd_networking_loadbalancer_test.go +++ b/hack/generated/controllers/crd_networking_loadbalancer_test.go @@ -13,6 +13,8 @@ import ( . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/Azure/go-autorest/autorest/to" + network "github.com/Azure/azure-service-operator/hack/generated/_apis/microsoft.network/v1alpha1api20201101" "github.com/Azure/azure-service-operator/hack/generated/pkg/genruntime" "github.com/Azure/azure-service-operator/hack/generated/pkg/testcommon" @@ -29,9 +31,9 @@ func Test_LoadBalancer_CRUD(t *testing.T) { // Public IP Address sku := network.PublicIPAddressSkuNameStandard - publicIPAddress := &network.PublicIPAddresses{ + publicIPAddress := &network.PublicIPAddress{ TypeMeta: metav1.TypeMeta{ - Kind: reflect.TypeOf(network.PublicIPAddresses{}).Name(), + Kind: reflect.TypeOf(network.PublicIPAddress{}).Name(), }, ObjectMeta: tc.MakeObjectMetaWithName(tc.Namer.GenerateName("publicip")), Spec: network.PublicIPAddresses_Spec{ @@ -40,7 +42,7 @@ func Test_LoadBalancer_CRUD(t *testing.T) { Sku: &network.PublicIPAddressSku{ Name: &sku, }, - PublicIPAllocationMethod: network.PublicIPAddressesSpecPropertiesPublicIPAllocationMethodStatic, + PublicIPAllocationMethod: network.PublicIPAddressPropertiesFormatPublicIPAllocationMethodStatic, }, } @@ -50,6 +52,7 @@ func Test_LoadBalancer_CRUD(t *testing.T) { loadBalancerSku := network.LoadBalancerSkuNameStandard lbName := tc.Namer.GenerateName("loadbalancer") lbFrontendName := "LoadBalancerFrontend" + protocol := network.InboundNatPoolPropertiesFormatProtocolTcp loadBalancer := &network.LoadBalancer{ ObjectMeta: tc.MakeObjectMetaWithName(lbName), Spec: network.LoadBalancers_Spec{ @@ -58,32 +61,28 @@ func Test_LoadBalancer_CRUD(t *testing.T) { Sku: &network.LoadBalancerSku{ Name: &loadBalancerSku, }, - FrontendIPConfigurations: []network.FrontendIPConfiguration{ + FrontendIPConfigurations: []network.LoadBalancers_Spec_Properties_FrontendIPConfigurations{ { Name: lbFrontendName, - Properties: &network.FrontendIPConfigurationPropertiesFormat{ - PublicIPAddress: &network.SubResource{ - Reference: tc.MakeReferenceFromResource(publicIPAddress), - }, + PublicIPAddress: &network.SubResource{ + Reference: tc.MakeReferenceFromResource(publicIPAddress), }, }, }, // TODO: The below stuff isn't really necessary for LB CRUD but is required for VMSS... - InboundNatPools: []network.InboundNatPool{ + InboundNatPools: []network.LoadBalancers_Spec_Properties_InboundNatPools{ { Name: "MyFancyNatPool", - Properties: &network.InboundNatPoolPropertiesFormat{ - FrontendIPConfiguration: network.SubResource{ - Reference: genruntime.ResourceReference{ - // TODO: This is still really awkward - ARMID: tc.MakeARMId(rg.Name, "Microsoft.Network", "loadBalancers", lbName, "frontendIPConfigurations", lbFrontendName), - }, + FrontendIPConfiguration: &network.SubResource{ + Reference: genruntime.ResourceReference{ + // TODO: This is still really awkward + ARMID: tc.MakeARMId(rg.Name, "Microsoft.Network", "loadBalancers", lbName, "frontendIPConfigurations", lbFrontendName), }, - Protocol: network.InboundNatPoolPropertiesFormatProtocolTcp, - FrontendPortRangeStart: 50000, - FrontendPortRangeEnd: 51000, - BackendPort: 22, }, + Protocol: &protocol, + FrontendPortRangeStart: to.IntPtr(50_000), + FrontendPortRangeEnd: to.IntPtr(51_000), + BackendPort: to.IntPtr(22), }, }, }, diff --git a/hack/generated/controllers/crd_networking_publicip_test.go b/hack/generated/controllers/crd_networking_publicip_test.go index 735d0befb63..f89688e1e81 100644 --- a/hack/generated/controllers/crd_networking_publicip_test.go +++ b/hack/generated/controllers/crd_networking_publicip_test.go @@ -26,7 +26,7 @@ func Test_PublicIP_CRUD(t *testing.T) { // TODO: Note the microsoft.networking package also defines a PublicIPAddress type, so // TODO: depluralization of this resource doesn't work because of the collision. sku := network.PublicIPAddressSkuNameStandard - publicIPAddress := &network.PublicIPAddresses{ + publicIPAddress := &network.PublicIPAddress{ ObjectMeta: tc.MakeObjectMetaWithName(tc.Namer.GenerateName("publicip")), Spec: network.PublicIPAddresses_Spec{ Location: tc.AzureRegion, @@ -34,7 +34,7 @@ func Test_PublicIP_CRUD(t *testing.T) { Sku: &network.PublicIPAddressSku{ Name: &sku, }, - PublicIPAllocationMethod: network.PublicIPAddressesSpecPropertiesPublicIPAllocationMethodStatic, + PublicIPAllocationMethod: network.PublicIPAddressPropertiesFormatPublicIPAllocationMethodStatic, }, } @@ -55,7 +55,7 @@ func Test_PublicIP_CRUD(t *testing.T) { // ensure state got updated in Azure tc.Eventually(func() *int { - updatedIP := &network.PublicIPAddresses{} + updatedIP := &network.PublicIPAddress{} tc.GetResource(objectKey, updatedIP) return updatedIP.Status.IdleTimeoutInMinutes }).Should(Equal(&idleTimeoutInMinutes)) diff --git a/hack/generated/controllers/crd_storageaccount_test.go b/hack/generated/controllers/crd_storageaccount_test.go index e111b6f0be3..d2952a8024e 100644 --- a/hack/generated/controllers/crd_storageaccount_test.go +++ b/hack/generated/controllers/crd_storageaccount_test.go @@ -33,8 +33,8 @@ func Test_StorageAccount_CRUD(t *testing.T) { Location: tc.AzureRegion, Owner: testcommon.AsOwner(rg.ObjectMeta), Kind: storage.StorageAccountsSpecKindBlobStorage, - Sku: storage.StorageAccounts_Spec_Sku{ - Name: storage.StorageAccountsSpecSkuNameStandardLRS, + Sku: storage.Sku{ + Name: storage.SkuNameStandardLRS, }, // TODO: They mark this property as optional but actually it is required AccessTier: &accessTier, @@ -100,7 +100,6 @@ func StorageAccount_BlobServices_CRUD(tc testcommon.KubePerTestContext, storageA } func StorageAccount_BlobServices_Container_CRUD(tc testcommon.KubePerTestContext, blobService metav1.ObjectMeta) { - blobContainer := &storage.StorageAccountsBlobServicesContainer{ ObjectMeta: tc.MakeObjectMeta("container"), Spec: storage.StorageAccountsBlobServicesContainers_Spec{ diff --git a/hack/generated/controllers/crd_vmss_test.go b/hack/generated/controllers/crd_vmss_test.go index e85b4b184f3..30769d135d4 100644 --- a/hack/generated/controllers/crd_vmss_test.go +++ b/hack/generated/controllers/crd_vmss_test.go @@ -9,15 +9,16 @@ import ( "fmt" "testing" + "github.com/Azure/go-autorest/autorest/to" + . "github.com/onsi/gomega" + v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + compute "github.com/Azure/azure-service-operator/hack/generated/_apis/microsoft.compute/v1alpha1api20201201" network "github.com/Azure/azure-service-operator/hack/generated/_apis/microsoft.network/v1alpha1api20201101" resources "github.com/Azure/azure-service-operator/hack/generated/_apis/microsoft.resources/v1alpha1api20200601" "github.com/Azure/azure-service-operator/hack/generated/pkg/genruntime" "github.com/Azure/azure-service-operator/hack/generated/pkg/testcommon" - "github.com/Azure/go-autorest/autorest/to" - . "github.com/onsi/gomega" - "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "sigs.k8s.io/controller-runtime/pkg/client" ) func newVNETForVMSS(tc testcommon.KubePerTestContext, owner genruntime.KnownResourceReference) *network.VirtualNetwork { @@ -43,9 +44,9 @@ func newSubnetForVMSS(tc testcommon.KubePerTestContext, owner genruntime.KnownRe } } -func newPublicIPAddressForVMSS(tc testcommon.KubePerTestContext, owner genruntime.KnownResourceReference) *network.PublicIPAddresses { +func newPublicIPAddressForVMSS(tc testcommon.KubePerTestContext, owner genruntime.KnownResourceReference) *network.PublicIPAddress { publicIPAddressSku := network.PublicIPAddressSkuNameStandard - return &network.PublicIPAddresses{ + return &network.PublicIPAddress{ ObjectMeta: tc.MakeObjectMetaWithName(tc.Namer.GenerateName("publicip")), Spec: network.PublicIPAddresses_Spec{ Location: tc.AzureRegion, @@ -53,15 +54,16 @@ func newPublicIPAddressForVMSS(tc testcommon.KubePerTestContext, owner genruntim Sku: &network.PublicIPAddressSku{ Name: &publicIPAddressSku, }, - PublicIPAllocationMethod: network.PublicIPAddressesSpecPropertiesPublicIPAllocationMethodStatic, + PublicIPAllocationMethod: network.PublicIPAddressPropertiesFormatPublicIPAllocationMethodStatic, }, } } -func newLoadBalancerForVMSS(tc testcommon.KubePerTestContext, rg *resources.ResourceGroup, publicIPAddress *network.PublicIPAddresses) *network.LoadBalancer { +func newLoadBalancerForVMSS(tc testcommon.KubePerTestContext, rg *resources.ResourceGroup, publicIPAddress *network.PublicIPAddress) *network.LoadBalancer { loadBalancerSku := network.LoadBalancerSkuNameStandard lbName := tc.Namer.GenerateName("loadbalancer") lbFrontendName := "LoadBalancerFrontend" + protocol := network.InboundNatPoolPropertiesFormatProtocolTcp return &network.LoadBalancer{ ObjectMeta: tc.MakeObjectMetaWithName(lbName), Spec: network.LoadBalancers_Spec{ @@ -70,31 +72,27 @@ func newLoadBalancerForVMSS(tc testcommon.KubePerTestContext, rg *resources.Reso Sku: &network.LoadBalancerSku{ Name: &loadBalancerSku, }, - FrontendIPConfigurations: []network.FrontendIPConfiguration{ + FrontendIPConfigurations: []network.LoadBalancers_Spec_Properties_FrontendIPConfigurations{ { Name: lbFrontendName, - Properties: &network.FrontendIPConfigurationPropertiesFormat{ - PublicIPAddress: &network.SubResource{ - Reference: tc.MakeReferenceFromResource(publicIPAddress), - }, + PublicIPAddress: &network.SubResource{ + Reference: tc.MakeReferenceFromResource(publicIPAddress), }, }, }, - InboundNatPools: []network.InboundNatPool{ + InboundNatPools: []network.LoadBalancers_Spec_Properties_InboundNatPools{ { Name: "MyFancyNatPool", - Properties: &network.InboundNatPoolPropertiesFormat{ - FrontendIPConfiguration: network.SubResource{ - Reference: genruntime.ResourceReference{ - // TODO: Getting this is SUPER awkward - ARMID: tc.MakeARMId(rg.Name, "Microsoft.Network", "loadBalancers", lbName, "frontendIPConfigurations", lbFrontendName), - }, + FrontendIPConfiguration: &network.SubResource{ + Reference: genruntime.ResourceReference{ + // TODO: Getting this is SUPER awkward + ARMID: tc.MakeARMId(rg.Name, "Microsoft.Network", "loadBalancers", lbName, "frontendIPConfigurations", lbFrontendName), }, - Protocol: network.InboundNatPoolPropertiesFormatProtocolTcp, - FrontendPortRangeStart: 50000, - FrontendPortRangeEnd: 51000, - BackendPort: 22, }, + Protocol: &protocol, + FrontendPortRangeStart: to.IntPtr(50_000), + FrontendPortRangeEnd: to.IntPtr(51_000), + BackendPort: to.IntPtr(22), }, }, }, @@ -132,7 +130,7 @@ func newVMSS( UpgradePolicy: &compute.UpgradePolicy{ Mode: &upgradePolicyMode, }, - VirtualMachineProfile: &compute.VirtualMachineScaleSetVMProfile{ + VirtualMachineProfile: &compute.VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile{ StorageProfile: &compute.VirtualMachineScaleSetStorageProfile{ ImageReference: &compute.ImageReference{ Publisher: to.StringPtr("Canonical"), @@ -156,24 +154,20 @@ func newVMSS( }, }, }, - NetworkProfile: &compute.VirtualMachineScaleSetNetworkProfile{ - NetworkInterfaceConfigurations: []compute.VirtualMachineScaleSetNetworkConfiguration{ + NetworkProfile: &compute.VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile_NetworkProfile{ + NetworkInterfaceConfigurations: []compute.VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile_NetworkProfile_NetworkInterfaceConfigurations{ { - Name: "mynicconfig", - Properties: &compute.VirtualMachineScaleSetNetworkConfigurationProperties{ - Primary: to.BoolPtr(true), - IpConfigurations: []compute.VirtualMachineScaleSetIPConfiguration{ - { - Name: "myipconfiguration", - Properties: &compute.VirtualMachineScaleSetIPConfigurationProperties{ - Subnet: &compute.ApiEntityReference{ - Reference: tc.MakeReferencePtrFromResource(subnet), - }, - LoadBalancerInboundNatPools: []compute.SubResource{ - { - Reference: &inboundNATPoolRef, - }, - }, + Name: "mynicconfig", + Primary: to.BoolPtr(true), + IpConfigurations: []compute.VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile_NetworkProfile_NetworkInterfaceConfigurations_Properties_IpConfigurations{ + { + Name: "myipconfiguration", + Subnet: &compute.ApiEntityReference{ + Reference: tc.MakeReferencePtrFromResource(subnet), + }, + LoadBalancerInboundNatPools: []compute.SubResource{ + { + Reference: &inboundNATPoolRef, }, }, }, @@ -206,18 +200,16 @@ func Test_VMSS_CRUD(t *testing.T) { // Perform a simple patch to add a basic custom script extension patcher := tc.NewResourcePatcher(vmss) extensionName := "mycustomextension" - vmss.Spec.VirtualMachineProfile.ExtensionProfile = &compute.VirtualMachineScaleSetExtensionProfile{ - Extensions: []compute.VirtualMachineScaleSetExtension{ + vmss.Spec.VirtualMachineProfile.ExtensionProfile = &compute.VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile_ExtensionProfile{ + Extensions: []compute.VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile_ExtensionProfile_Extensions{ { - Name: &extensionName, - Properties: &compute.GenericExtension{ - Publisher: "Microsoft.Azure.Extensions", - Type: "CustomScript", - TypeHandlerVersion: "2.0", - Settings: map[string]v1.JSON{ - "commandToExecute": { - Raw: []byte(`"/bin/bash -c \"echo hello\""`), - }, + Name: &extensionName, + Publisher: to.StringPtr("Microsoft.Azure.Extensions"), + Type: to.StringPtr("CustomScript"), + TypeHandlerVersion: to.StringPtr("2.0"), + Settings: map[string]v1.JSON{ + "commandToExecute": { + Raw: []byte(`"/bin/bash -c \"echo hello\""`), }, }, }, diff --git a/hack/generated/controllers/deployment_reconciler_negative_test.go b/hack/generated/controllers/deployment_reconciler_negative_test.go index e0166cdf8d6..b70cfbfb629 100644 --- a/hack/generated/controllers/deployment_reconciler_negative_test.go +++ b/hack/generated/controllers/deployment_reconciler_negative_test.go @@ -11,13 +11,14 @@ import ( . "github.com/onsi/gomega" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/Azure/go-autorest/autorest/to" + compute "github.com/Azure/azure-service-operator/hack/generated/_apis/microsoft.compute/v1alpha1api20201201" resources "github.com/Azure/azure-service-operator/hack/generated/_apis/microsoft.resources/v1alpha1api20200601" storage "github.com/Azure/azure-service-operator/hack/generated/_apis/microsoft.storage/v1alpha1api20210401" "github.com/Azure/azure-service-operator/hack/generated/pkg/armclient" "github.com/Azure/azure-service-operator/hack/generated/pkg/reconcilers" "github.com/Azure/azure-service-operator/hack/generated/pkg/testcommon" - "github.com/Azure/go-autorest/autorest/to" ) func newStorageAccountWithInvalidKeyExpiration(tc testcommon.KubePerTestContext, rg *resources.ResourceGroup) *storage.StorageAccount { @@ -32,8 +33,8 @@ func newStorageAccountWithInvalidKeyExpiration(tc testcommon.KubePerTestContext, Location: tc.AzureRegion, Owner: testcommon.AsOwner(rg.ObjectMeta), Kind: storage.StorageAccountsSpecKindBlobStorage, - Sku: storage.StorageAccounts_Spec_Sku{ - Name: storage.StorageAccountsSpecSkuNameStandardLRS, + Sku: storage.Sku{ + Name: storage.SkuNameStandardLRS, }, AccessTier: &accessTier, KeyPolicy: &storage.KeyPolicy{ @@ -60,7 +61,7 @@ func newVMSSWithInvalidPublisher(tc testcommon.KubePerTestContext, rg *resources UpgradePolicy: &compute.UpgradePolicy{ Mode: &upgradePolicyMode, }, - VirtualMachineProfile: &compute.VirtualMachineScaleSetVMProfile{ + VirtualMachineProfile: &compute.VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile{ StorageProfile: &compute.VirtualMachineScaleSetStorageProfile{ ImageReference: &compute.ImageReference{ Publisher: to.StringPtr("this publisher"), @@ -73,8 +74,8 @@ func newVMSSWithInvalidPublisher(tc testcommon.KubePerTestContext, rg *resources ComputerNamePrefix: to.StringPtr("computer"), AdminUsername: &adminUsername, }, - NetworkProfile: &compute.VirtualMachineScaleSetNetworkProfile{ - NetworkInterfaceConfigurations: []compute.VirtualMachineScaleSetNetworkConfiguration{ + NetworkProfile: &compute.VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile_NetworkProfile{ + NetworkInterfaceConfigurations: []compute.VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile_NetworkProfile_NetworkInterfaceConfigurations{ { Name: "mynicconfig", }, diff --git a/hack/generated/controllers/edge_case_test.go b/hack/generated/controllers/edge_case_test.go index ba4b4b7a0dd..6f7f0cd27a8 100644 --- a/hack/generated/controllers/edge_case_test.go +++ b/hack/generated/controllers/edge_case_test.go @@ -8,15 +8,16 @@ package controllers_test import ( "testing" + . "github.com/onsi/gomega" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + network "github.com/Azure/azure-service-operator/hack/generated/_apis/microsoft.network/v1alpha1api20201101" resources "github.com/Azure/azure-service-operator/hack/generated/_apis/microsoft.resources/v1alpha1api20200601" storage "github.com/Azure/azure-service-operator/hack/generated/_apis/microsoft.storage/v1alpha1api20210401" "github.com/Azure/azure-service-operator/hack/generated/pkg/reconcilers" "github.com/Azure/azure-service-operator/hack/generated/pkg/testcommon" - . "github.com/onsi/gomega" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) func waitForOwnerMissingError(tc testcommon.KubePerTestContext, obj controllerutil.Object) { @@ -50,8 +51,8 @@ func storageAccountAndResourceGroupProvisionedOutOfOrderHelper(t *testing.T, wai Location: tc.AzureRegion, Owner: testcommon.AsOwner(rg.ObjectMeta), Kind: storage.StorageAccountsSpecKindBlobStorage, - Sku: storage.StorageAccounts_Spec_Sku{ - Name: storage.StorageAccountsSpecSkuNameStandardLRS, + Sku: storage.Sku{ + Name: storage.SkuNameStandardLRS, }, AccessTier: &accessTier, }, @@ -160,8 +161,8 @@ func Test_CreateStorageAccountThatAlreadyExists_ReconcilesSuccessfully(t *testin Location: tc.AzureRegion, Owner: testcommon.AsOwner(rg.ObjectMeta), Kind: storage.StorageAccountsSpecKindBlobStorage, - Sku: storage.StorageAccounts_Spec_Sku{ - Name: storage.StorageAccountsSpecSkuNameStandardLRS, + Sku: storage.Sku{ + Name: storage.SkuNameStandardLRS, }, AccessTier: &accessTier, }, diff --git a/hack/generator/azure-arm.yaml b/hack/generator/azure-arm.yaml index 4a206477aa3..b99173d64b6 100644 --- a/hack/generator/azure-arm.yaml +++ b/hack/generator/azure-arm.yaml @@ -152,7 +152,7 @@ exportFilters: - action: include-transitive group: microsoft.network version: v*api20201101 - name: PublicIPAddresses + name: PublicIPAddress because: "including public IP Address" - action: include-transitive group: microsoft.network diff --git a/hack/generator/pkg/astmodel/allof_type.go b/hack/generator/pkg/astmodel/allof_type.go index c5d9f51a50b..3f0efe279f7 100644 --- a/hack/generator/pkg/astmodel/allof_type.go +++ b/hack/generator/pkg/astmodel/allof_type.go @@ -134,15 +134,15 @@ func (allOf AllOfType) RequiredPackageReferences() *PackageReferenceSet { // Equals returns true if the other Type is a AllOf that contains // the same set of types func (allOf *AllOfType) Equals(t Type) bool { + if allOf == t { + return true // short-circuit + } + other, ok := t.(*AllOfType) if !ok { return false } - if allOf == other { - return true // short-circuit - } - return allOf.types.Equals(other.types) } diff --git a/hack/generator/pkg/astmodel/enum_type.go b/hack/generator/pkg/astmodel/enum_type.go index a61d7b61a77..375506dcf65 100644 --- a/hack/generator/pkg/astmodel/enum_type.go +++ b/hack/generator/pkg/astmodel/enum_type.go @@ -11,9 +11,10 @@ import ( "sort" "strings" - "github.com/Azure/azure-service-operator/hack/generator/pkg/astbuilder" "github.com/dave/dst" "k8s.io/klog/v2" + + "github.com/Azure/azure-service-operator/hack/generator/pkg/astbuilder" ) // EnumType represents a set of mutually exclusive predefined options @@ -95,7 +96,6 @@ func (enum *EnumType) createBaseDeclaration( } func (enum *EnumType) createValueDeclaration(name TypeName, value EnumValue) dst.Spec { - valueSpec := &dst.ValueSpec{ Names: []*dst.Ident{dst.NewIdent(GetEnumValueId(name.name, value))}, Values: []dst.Expr{ @@ -126,6 +126,10 @@ func (enum *EnumType) References() TypeNameSet { // Equals will return true if the supplied type has the same base type and options func (enum *EnumType) Equals(t Type) bool { + if enum == t { + return true // short-circuit + } + if e, ok := t.(*EnumType); ok { if !enum.baseType.Equals(e.baseType) { return false diff --git a/hack/generator/pkg/astmodel/errored_type.go b/hack/generator/pkg/astmodel/errored_type.go index 44f70b81955..16d544f893d 100644 --- a/hack/generator/pkg/astmodel/errored_type.go +++ b/hack/generator/pkg/astmodel/errored_type.go @@ -53,25 +53,47 @@ func (e *ErroredType) WithType(t Type) *ErroredType { } func (e *ErroredType) Equals(t Type) bool { + if e == t { + return true // short-circuit + } + other, ok := t.(*ErroredType) if !ok { return false } - if e == other { - return true // short-circuit - } - return ((e.inner == nil && other.inner == nil) || e.inner.Equals(other.inner)) && stringSlicesEqual(e.warnings, other.warnings) && stringSlicesEqual(e.errors, other.errors) } -func stringSlicesEqual(l []string, r []string) bool { +func propertyNameSlicesEqual(l, r []PropertyName) bool { + if len(l) != len(r) { + return false + } + + if len(l) > 0 && (&l[0] == &r[0]) { + return true // reference equality + } + + for ix := range l { + if string(l[ix]) != string(r[ix]) { + return false + } + } + + return true +} + +func stringSlicesEqual(l, r []string) bool { if len(l) != len(r) { return false } + if len(l) > 0 && (&l[0] == &r[0]) { + return true // reference equality + } + for ix := range l { if l[ix] != r[ix] { return false diff --git a/hack/generator/pkg/astmodel/flagged_type.go b/hack/generator/pkg/astmodel/flagged_type.go index 6ecf45eda15..96bd368c7f0 100644 --- a/hack/generator/pkg/astmodel/flagged_type.go +++ b/hack/generator/pkg/astmodel/flagged_type.go @@ -124,6 +124,10 @@ func (ft *FlaggedType) AsZero(types Types, ctx *CodeGenerationContext) dst.Expr // Equals returns true if the passed type is the same as this one, false otherwise func (ft *FlaggedType) Equals(t Type) bool { + if ft == t { + return true // short-circuit + } + other, ok := t.(*FlaggedType) if !ok { return false diff --git a/hack/generator/pkg/astmodel/map_type.go b/hack/generator/pkg/astmodel/map_type.go index a9fc0532bad..2701a619ff2 100644 --- a/hack/generator/pkg/astmodel/map_type.go +++ b/hack/generator/pkg/astmodel/map_type.go @@ -76,7 +76,7 @@ func (m *MapType) References() TypeNameSet { // Equals returns true if the passed type is a map type with the same kinds of keys and elements, false otherwise func (m *MapType) Equals(t Type) bool { if m == t { - return true + return true // short-circuit } if mt, ok := t.(*MapType); ok { diff --git a/hack/generator/pkg/astmodel/object_type.go b/hack/generator/pkg/astmodel/object_type.go index 9bc14faeced..c1a09f03f47 100644 --- a/hack/generator/pkg/astmodel/object_type.go +++ b/hack/generator/pkg/astmodel/object_type.go @@ -89,7 +89,6 @@ func (objectType *ObjectType) generateMethodDecls(codeGenerationContext *CodeGen } func defineField(fieldName string, fieldType dst.Expr, tag string) *dst.Field { - result := &dst.Field{ Type: fieldType, Tag: astbuilder.TextLiteral(tag), @@ -154,7 +153,6 @@ func (objectType *ObjectType) EmbeddedProperties() []*PropertyDefinition { // Functions returns all the function implementations // A sorted slice is returned to preserve immutability and provide determinism func (objectType *ObjectType) Functions() []Function { - functions := make([]Function, 0, len(objectType.functions)) for _, f := range objectType.functions { functions = append(functions, f) @@ -251,7 +249,7 @@ func (objectType *ObjectType) References() TypeNameSet { // The order of the properties is not relevant func (objectType *ObjectType) Equals(t Type) bool { if objectType == t { - return true + return true // short circuit } other, ok := t.(*ObjectType) @@ -376,7 +374,6 @@ func (objectType *ObjectType) WithProperties(properties ...*PropertyDefinition) // WithEmbeddedProperties creates a new ObjectType with the additional embedded properties included func (objectType *ObjectType) WithEmbeddedProperties(properties ...*PropertyDefinition) (*ObjectType, error) { - // Create a copy of objectType to preserve immutability result := objectType.copy() diff --git a/hack/generator/pkg/astmodel/oneof_type.go b/hack/generator/pkg/astmodel/oneof_type.go index cbcf199fee5..54407e93353 100644 --- a/hack/generator/pkg/astmodel/oneof_type.go +++ b/hack/generator/pkg/astmodel/oneof_type.go @@ -97,15 +97,15 @@ func (oneOf OneOfType) RequiredPackageReferences() *PackageReferenceSet { // Equals returns true if the other Type is a OneOfType that contains // the same set of types func (oneOf *OneOfType) Equals(t Type) bool { + if oneOf == t { + return true // short-circuit + } + other, ok := t.(*OneOfType) if !ok { return false } - if oneOf == other { - return true // short-circuit - } - return oneOf.types.Equals(other.types) } diff --git a/hack/generator/pkg/astmodel/primitive_type.go b/hack/generator/pkg/astmodel/primitive_type.go index cc1fcf71aa4..f52a75bec1c 100644 --- a/hack/generator/pkg/astmodel/primitive_type.go +++ b/hack/generator/pkg/astmodel/primitive_type.go @@ -6,8 +6,9 @@ package astmodel import ( - "github.com/dave/dst" "strings" + + "github.com/dave/dst" ) // PrimitiveType represents a Go primitive type @@ -74,6 +75,10 @@ func (prim *PrimitiveType) References() TypeNameSet { // Equals returns true if the passed type is another primitive type the same name, false otherwise func (prim *PrimitiveType) Equals(t Type) bool { + if prim == t { + return true // short circuit + } + if p, ok := t.(*PrimitiveType); ok { return prim.name == p.name } diff --git a/hack/generator/pkg/astmodel/property_definition.go b/hack/generator/pkg/astmodel/property_definition.go index de4eea61eff..7301fb29880 100644 --- a/hack/generator/pkg/astmodel/property_definition.go +++ b/hack/generator/pkg/astmodel/property_definition.go @@ -134,7 +134,7 @@ func (property *PropertyDefinition) WithType(newType Type) *PropertyDefinition { panic("nil type provided to WithType") } - if property.propertyType.Equals(newType) { + if property.propertyType == newType || property.propertyType.Equals(newType) { return property } @@ -261,7 +261,14 @@ func (property *PropertyDefinition) MakeOptional() *PropertyDefinition { // whereas in the MakeRequired direction we only care if the type is specifically *astmodel.Optional if !isTypeOptional(property.propertyType) { // Need to make the type optional - result.propertyType = NewOptionalType(result.propertyType) + + // we must check if the type is validated to preserve the validation invariant + // that all validations are *directly* under either a named type or a property + if vType, ok := result.propertyType.(*ValidatedType); ok { + result.propertyType = NewValidatedType(NewOptionalType(vType.ElementType()), vType.validations) + } else { + result.propertyType = NewOptionalType(result.propertyType) + } } result.hasKubebuilderRequiredValidation = false @@ -309,6 +316,10 @@ func (property *PropertyDefinition) renderedTags() string { // AsField generates a Go AST field node representing this property definition func (property *PropertyDefinition) AsField(codeGenerationContext *CodeGenerationContext) *dst.Field { + if property.flatten { + panic(fmt.Sprintf("property %s marked for flattening was not flattened", property.propertyName)) + } + tags := property.renderedTags() var names []*dst.Ident @@ -385,12 +396,14 @@ func (property *PropertyDefinition) tagsEqual(f *PropertyDefinition) bool { } // Equals tests to see if the specified PropertyDefinition specifies the same property -func (property *PropertyDefinition) Equals(f *PropertyDefinition) bool { - return property == f || (property.propertyName == f.propertyName && - property.propertyType.Equals(f.propertyType) && - property.tagsEqual(f) && - property.hasKubebuilderRequiredValidation == f.hasKubebuilderRequiredValidation && - property.description == f.description) +func (property *PropertyDefinition) Equals(o *PropertyDefinition) bool { + return property == o || (property.propertyName == o.propertyName && + property.propertyType.Equals(o.propertyType) && + property.flatten == o.flatten && + propertyNameSlicesEqual(property.flattenedFrom, o.flattenedFrom) && + property.tagsEqual(o) && + property.hasKubebuilderRequiredValidation == o.hasKubebuilderRequiredValidation && + property.description == o.description) } func (property *PropertyDefinition) copy() *PropertyDefinition { diff --git a/hack/generator/pkg/astmodel/property_definition_test.go b/hack/generator/pkg/astmodel/property_definition_test.go index 72e20dad3df..18e657776f1 100644 --- a/hack/generator/pkg/astmodel/property_definition_test.go +++ b/hack/generator/pkg/astmodel/property_definition_test.go @@ -334,7 +334,6 @@ func Test_PropertyDefinitionMakeRequired_WhenTypeOptionalAndIsRequired_ReturnsNe } func Test_PropertyDefinitionMakeRequired_PropertyTypeArrayAndMap(t *testing.T) { - cases := []struct { name string propertyType Type @@ -403,7 +402,6 @@ func Test_PropertyDefinitionMakeOptional_WhenTypeMandatoryAndIsRequiredFalse_Ret } func Test_PropertyDefinitionMakeOptional_PropertyTypeArrayAndMap(t *testing.T) { - cases := []struct { name string propertyType Type @@ -476,7 +474,6 @@ func Test_PropertyDefinition_WithValidation_ReturnsNewPropertyDefinition(t *test */ func TestPropertyDefinition_Equals_WhenGivenPropertyDefinition_ReturnsExpectedResult(t *testing.T) { - strProperty := createStringProperty("FullName", "Full Legal Name") otherStrProperty := createStringProperty("FullName", "Full Legal Name") @@ -548,3 +545,22 @@ func TestSettingSameFlattenValueDoesNotAllocateNewPropertyDefinition(t *testing. g.Expect(strProperty.SetFlatten(false)).To(BeIdenticalTo(strProperty)) g.Expect(strPropertyFlatten.SetFlatten(true)).To(BeIdenticalTo(strPropertyFlatten)) } + +func TestPropertyDefinitionsWithDifferentFlattenSettingsAreNotEqual(t *testing.T) { + g := NewGomegaWithT(t) + + strProperty := createStringProperty("FullName", "Full Legal Name") + strPropertyFlatten := strProperty.SetFlatten(true) + g.Expect(strProperty.Equals(strPropertyFlatten)).To(BeFalse()) +} + +func TestPropertyDefinitionsWithDifferentFlattenedFromSettingsAreNotEqual(t *testing.T) { + g := NewGomegaWithT(t) + + strProperty := createStringProperty("FullName", "Full Legal Name") + strPropertyFlatten1 := strProperty.AddFlattenedFrom("foo") + strPropertyFlatten2 := strProperty.AddFlattenedFrom("bar") + g.Expect(strProperty.Equals(strPropertyFlatten1)).To(BeFalse()) + g.Expect(strProperty.Equals(strPropertyFlatten2)).To(BeFalse()) + g.Expect(strPropertyFlatten1.Equals(strPropertyFlatten2)).To(BeFalse()) +} diff --git a/hack/generator/pkg/astmodel/resource_type.go b/hack/generator/pkg/astmodel/resource_type.go index 1760fbcec64..b6fdcf7c605 100644 --- a/hack/generator/pkg/astmodel/resource_type.go +++ b/hack/generator/pkg/astmodel/resource_type.go @@ -153,7 +153,6 @@ func (resource *ResourceType) IsStorageVersion() bool { // WithSpec returns a new resource that has the specified spec type func (resource *ResourceType) WithSpec(specType Type) *ResourceType { - if specResource, ok := specType.(*ResourceType); ok { // type is a resource, take its SpecType instead // so we don't nest resources @@ -167,7 +166,6 @@ func (resource *ResourceType) WithSpec(specType Type) *ResourceType { // WithStatus returns a new resource that has the specified status type func (resource *ResourceType) WithStatus(statusType Type) *ResourceType { - if specResource, ok := statusType.(*ResourceType); ok { // type is a resource, take its StatusType instead // so we don't nest resources @@ -286,7 +284,6 @@ func (resource *ResourceType) Equals(other Type) bool { // EmbeddedProperties returns all the embedded properties for this resource type // An ordered slice is returned to preserve immutability and provide determinism func (resource *ResourceType) EmbeddedProperties() []*PropertyDefinition { - typeMetaType := MakeTypeName(MetaV1PackageReference, "TypeMeta") typeMetaProperty := NewPropertyDefinition("", "", typeMetaType). WithTag("json", "inline") @@ -331,7 +328,6 @@ func (resource *ResourceType) WithoutProperty(name PropertyName) *ResourceType { // Properties returns all the properties from this resource type // An ordered slice is returned to preserve immutability and provide determinism func (resource *ResourceType) Properties() []*PropertyDefinition { - result := []*PropertyDefinition{ resource.createSpecProperty(), } @@ -380,7 +376,6 @@ func (resource *ResourceType) Property(name PropertyName) (*PropertyDefinition, // Functions returns all the function implementations // A sorted slice is returned to preserve immutability and provide determinism func (resource *ResourceType) Functions() []Function { - var functions []Function for _, f := range resource.functions { functions = append(functions, f) diff --git a/hack/generator/pkg/astmodel/type_definition.go b/hack/generator/pkg/astmodel/type_definition.go index 8f7a6a6be13..ab343c4c487 100644 --- a/hack/generator/pkg/astmodel/type_definition.go +++ b/hack/generator/pkg/astmodel/type_definition.go @@ -6,11 +6,13 @@ package astmodel import ( + "fmt" "go/token" - "github.com/Azure/azure-service-operator/hack/generator/pkg/astbuilder" "github.com/dave/dst" "github.com/pkg/errors" + + "github.com/Azure/azure-service-operator/hack/generator/pkg/astbuilder" ) // TypeDefinition is a name paired with a type @@ -73,6 +75,12 @@ func (def TypeDefinition) AsDeclarations(codeGenerationContext *CodeGenerationCo Description: def.description, } + defer func() { + if p := recover(); p != nil { + panic(fmt.Sprintf("while generating %s/%s: %s", codeGenerationContext.currentPackage, def.name, p)) + } + }() + return def.theType.AsDeclarations(codeGenerationContext, declContext) } diff --git a/hack/generator/pkg/astmodel/type_name.go b/hack/generator/pkg/astmodel/type_name.go index f853196bf37..27d7f559a84 100644 --- a/hack/generator/pkg/astmodel/type_name.go +++ b/hack/generator/pkg/astmodel/type_name.go @@ -69,7 +69,6 @@ func (typeName TypeName) AsType(codeGenerationContext *CodeGenerationContext) ds // AsZero renders an expression for the "zero" value of the type. // The exact thing we need to generate depends on the actual type we reference func (typeName TypeName) AsZero(types Types, ctx *CodeGenerationContext) dst.Expr { - if _, isLocal := typeName.PackageReference.AsLocalPackage(); !isLocal { // TypeName is external, zero value is a qualified empty struct // (we might not actually use this, if the property is optional, but we still need to generate the right thing) @@ -121,6 +120,10 @@ func (typeName TypeName) RequiredPackageReferences() *PackageReferenceSet { // Equals returns true if the passed type is the same TypeName, false otherwise func (typeName TypeName) Equals(t Type) bool { + if typeName == t { + return true + } + if d, ok := t.(TypeName); ok { return typeName.name == d.name && typeName.PackageReference.Equals(d.PackageReference) } diff --git a/hack/generator/pkg/astmodel/validated_type.go b/hack/generator/pkg/astmodel/validated_type.go index fb9d0d2bdd2..ca240e7cac9 100644 --- a/hack/generator/pkg/astmodel/validated_type.go +++ b/hack/generator/pkg/astmodel/validated_type.go @@ -194,6 +194,10 @@ func (v *ValidatedType) String() string { } func (v *ValidatedType) Equals(t Type) bool { + if v == t { + return true + } + other, ok := t.(*ValidatedType) if !ok { return false diff --git a/hack/generator/pkg/codegen/code_generator.go b/hack/generator/pkg/codegen/code_generator.go index 7fe8977a5c6..699b0c94166 100644 --- a/hack/generator/pkg/codegen/code_generator.go +++ b/hack/generator/pkg/codegen/code_generator.go @@ -79,7 +79,6 @@ func NewCodeGeneratorFromConfig(configuration *config.Configuration, idFactory a } func createAllPipelineStages(idFactory astmodel.IdentifierFactory, configuration *config.Configuration) []pipeline.Stage { - // graph keeps track of the conversions we need between different API & Storage versions graph := storage.NewConversionGraph() @@ -157,7 +156,7 @@ func createAllPipelineStages(idFactory astmodel.IdentifierFactory, configuration pipeline.AddCrossplaneEmbeddedResourceStatus(idFactory).UsedFor(pipeline.CrossplaneTarget), // Create Storage types - //TODO: For now only used for ARM + // TODO: For now only used for ARM pipeline.InjectOriginalVersionFunction(idFactory).UsedFor(pipeline.ARMTarget), pipeline.CreateStorageTypes(graph).UsedFor(pipeline.ARMTarget), pipeline.InjectOriginalVersionProperty().UsedFor(pipeline.ARMTarget), diff --git a/hack/generator/pkg/codegen/pipeline/add_cross_resource_references.go b/hack/generator/pkg/codegen/pipeline/add_cross_resource_references.go index 93e10963a31..424f6d7275c 100644 --- a/hack/generator/pkg/codegen/pipeline/add_cross_resource_references.go +++ b/hack/generator/pkg/codegen/pipeline/add_cross_resource_references.go @@ -28,7 +28,6 @@ func AddCrossResourceReferences(configuration *config.Configuration, idFactory a "addCrossResourceReferences", "Replace cross-resource references with genruntime.ResourceReference", func(ctx context.Context, definitions astmodel.Types) (astmodel.Types, error) { - result := make(astmodel.Types) knownReferences := newKnownReferencesMap(configuration) @@ -47,7 +46,7 @@ func AddCrossResourceReferences(configuration *config.Configuration, idFactory a // trust the Swagger. isCrossResourceReferenceErrs = append( isCrossResourceReferenceErrs, - errors.Errorf("\"%s.%s\" looks like a resource reference but was not labelled as one", typeName, prop.PropertyName())) + errors.Errorf("\"%s.%s\" looks like a resource reference but was not labelled as one. It might need to be manually added to `newKnownReferencesMap`", typeName, prop.PropertyName())) } return isReference @@ -151,7 +150,6 @@ func DoesPropertyLookLikeARMReference(prop *astmodel.PropertyDefinition) bool { } func makeResourceReferenceProperty(idFactory astmodel.IdentifierFactory, existing *astmodel.PropertyDefinition) *astmodel.PropertyDefinition { - var referencePropertyName string // Special case for "Id" and properties that end in "Id", which are quite common in the specs. This is primarily // because it's awkward to have a field called "Id" not just be a string and instead but a complex type describing @@ -225,7 +223,7 @@ func newKnownReferencesMap(configuration *config.Configuration) map[referencePai // It's never supplied in a PUT I don't think, and is only returned in a GET because the // IPConfiguration is actually an ARM resource that can only be created by issuing a PUT VMSS. { - typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20201201"), "VirtualMachineScaleSetIPConfiguration"), + typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20201201"), "VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile_NetworkProfile_NetworkInterfaceConfigurations_Properties_IpConfigurations"), propName: "Id", }: false, { @@ -240,7 +238,7 @@ func newKnownReferencesMap(configuration *config.Configuration) map[referencePai // It's never supplied in a PUT I don't think, and is only returned in a GET because the // IPConfiguration is actually an ARM resource that can only be created by issuing a PUT VMSS. { - typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20201201"), "VirtualMachineScaleSetNetworkConfiguration"), + typeName: astmodel.MakeTypeName(configuration.MakeLocalPackageReference("microsoft.compute", "v1alpha1api20201201"), "VirtualMachineScaleSets_Spec_Properties_VirtualMachineProfile_NetworkProfile_NetworkInterfaceConfigurations"), propName: "Id", }: false, // When SubResource is used directly in a property, it's meant as a reference. When it's inherited from, the Id is for self diff --git a/hack/generator/pkg/codegen/pipeline/convert_allof_and_oneof_to_objects.go b/hack/generator/pkg/codegen/pipeline/convert_allof_and_oneof_to_objects.go index c8a082493d6..6ebaf99ed82 100644 --- a/hack/generator/pkg/codegen/pipeline/convert_allof_and_oneof_to_objects.go +++ b/hack/generator/pkg/codegen/pipeline/convert_allof_and_oneof_to_objects.go @@ -149,7 +149,6 @@ func (ns propertyNames) betterThan(other propertyNames) bool { } func (s synthesizer) getOneOfPropNames(oneOf *astmodel.OneOfType) ([]propertyNames, error) { - var result []propertyNames err := oneOf.Types().ForEachError(func(t astmodel.Type, ix int) error { @@ -231,7 +230,6 @@ func (s synthesizer) getOneOfName(t astmodel.Type, propIndex int) (propertyNames return nil }) - if err != nil { return propertyNames{}, err } @@ -425,7 +423,7 @@ func (s synthesizer) handleObjectObject(leftObj *astmodel.ObjectType, rightObj * if err != nil { klog.Errorf("unable to combine properties: %s (%v)", p.PropertyName(), err) continue - //return nil, err + // return nil, err } // TODO: need to handle merging requiredness and tags and... @@ -515,7 +513,6 @@ func (s synthesizer) handleOneOf(leftOneOf *astmodel.OneOfType, right astmodel.T newTypes = append(newTypes, newType) return nil }) - if err != nil { return nil, err } @@ -590,14 +587,12 @@ func (synthesizer) handleMapObject(leftMap *astmodel.MapType, rightObj *astmodel // makes an ObjectType for an AllOf type func (s synthesizer) allOfObject(allOf *astmodel.AllOfType) (astmodel.Type, error) { - var intersection astmodel.Type = astmodel.AnyType err := allOf.Types().ForEachError(func(t astmodel.Type, _ int) error { var err error intersection, err = s.intersectTypes(intersection, t) return err }) - if err != nil { return nil, err } diff --git a/hack/generator/pkg/codegen/pipeline/create_arm_types.go b/hack/generator/pkg/codegen/pipeline/create_arm_types.go index 90119c147c7..f72fcf6e9f1 100644 --- a/hack/generator/pkg/codegen/pipeline/create_arm_types.go +++ b/hack/generator/pkg/codegen/pipeline/create_arm_types.go @@ -140,6 +140,10 @@ func removeValidations(t *astmodel.ObjectType) (*astmodel.ObjectType, error) { return t, nil } +func removeFlattening(t *astmodel.ObjectType) (*astmodel.ObjectType, error) { + return removeFlattenFromObject(t), nil +} + func (c *armTypeCreator) createARMTypeDefinition(isSpecType bool, def astmodel.TypeDefinition) (astmodel.TypeDefinition, error) { convertObjectPropertiesForARM := func(t *astmodel.ObjectType) (*astmodel.ObjectType, error) { return c.convertObjectPropertiesForARM(t, isSpecType) @@ -155,7 +159,7 @@ func (c *armTypeCreator) createARMTypeDefinition(isSpecType bool, def astmodel.T } armName := astmodel.CreateARMTypeName(def.Name()) - armDef, err := def.WithName(armName).ApplyObjectTransformations(removeValidations, convertObjectPropertiesForARM, addOneOfConversionFunctionIfNeeded) + armDef, err := def.WithName(armName).ApplyObjectTransformations(removeValidations, convertObjectPropertiesForARM, addOneOfConversionFunctionIfNeeded, removeFlattening) if err != nil { return astmodel.TypeDefinition{}, errors.Wrapf(err, "creating ARM prototype %v from Kubernetes definition %v", armName, def.Name()) diff --git a/hack/generator/pkg/codegen/pipeline/determine_resource_ownership.go b/hack/generator/pkg/codegen/pipeline/determine_resource_ownership.go index af9da624115..2cf585eec6c 100644 --- a/hack/generator/pkg/codegen/pipeline/determine_resource_ownership.go +++ b/hack/generator/pkg/codegen/pipeline/determine_resource_ownership.go @@ -27,7 +27,6 @@ func DetermineResourceOwnership(configuration *config.Configuration) Stage { } func determineOwnership(definitions astmodel.Types, configuration *config.Configuration) (astmodel.Types, error) { - updatedDefs := make(astmodel.Types) for _, def := range definitions { @@ -180,6 +179,9 @@ func extractChildResourceTypeNames(resourcesPropertyTypeDef astmodel.TypeDefinit } } +// this is the name we expect to see on "child resources" in the ARM JSON schema +const ChildResourceNameSuffix = "ChildResource" + func updateChildResourceDefinitionsWithOwner( definitions astmodel.Types, childResourceTypeNames []astmodel.TypeName, @@ -188,8 +190,8 @@ func updateChildResourceDefinitionsWithOwner( for _, typeName := range childResourceTypeNames { // If the typename ends in ChildResource, remove that - if strings.HasSuffix(typeName.Name(), "ChildResource") { - typeName = astmodel.MakeTypeName(typeName.PackageReference, strings.TrimSuffix(typeName.Name(), "ChildResource")) + if strings.HasSuffix(typeName.Name(), ChildResourceNameSuffix) { + typeName = astmodel.MakeTypeName(typeName.PackageReference, strings.TrimSuffix(typeName.Name(), ChildResourceNameSuffix)) } // If type typename is ExtensionsChild, remove Child -- this is a special case due to diff --git a/hack/generator/pkg/codegen/pipeline/flatten_properties.go b/hack/generator/pkg/codegen/pipeline/flatten_properties.go index 05faa279ef2..d2275df1b0f 100644 --- a/hack/generator/pkg/codegen/pipeline/flatten_properties.go +++ b/hack/generator/pkg/codegen/pipeline/flatten_properties.go @@ -56,7 +56,7 @@ func makeFlatteningVisitor(defs astmodel.Types) astmodel.TypeVisitor { // safety check: if err := checkForDuplicateNames(newProps); err != nil { klog.Warningf("Skipping flattening for %s: %s", name, err) - return it, nil // nolint:nilerr + return removeFlattenFromObject(it), nil } if len(newProps) != len(it.Properties()) { @@ -67,17 +67,28 @@ func makeFlatteningVisitor(defs astmodel.Types) astmodel.TypeVisitor { return result, nil }, - VisitFlaggedType: func(this *astmodel.TypeVisitor, it *astmodel.FlaggedType, ctx interface{}) (astmodel.Type, error) { - // skip ARM types, do not flatten - if it.HasFlag(astmodel.ARMFlag) { - return it, nil - } - - return astmodel.IdentityVisitOfFlaggedType(this, it, ctx) - }, }.Build() } +func removeFlattenFromObject(tObj *astmodel.ObjectType) *astmodel.ObjectType { + var props []*astmodel.PropertyDefinition + for _, prop := range tObj.Properties() { + prop = prop.WithType(removeFlatten(prop.PropertyType())) + prop = prop.SetFlatten(false) + props = append(props, prop) + } + + return tObj.WithProperties(props...) +} + +func removeFlatten(t astmodel.Type) astmodel.Type { + if tObj, ok := t.(*astmodel.ObjectType); ok { + return removeFlattenFromObject(tObj) + } + + return t +} + func checkForDuplicateNames(props []*astmodel.PropertyDefinition) error { names := make(map[astmodel.PropertyName]struct{}) for _, p := range props { diff --git a/hack/generator/pkg/codegen/pipeline/flatten_properties_test.go b/hack/generator/pkg/codegen/pipeline/flatten_properties_test.go index 762606543f7..b3e9908783f 100644 --- a/hack/generator/pkg/codegen/pipeline/flatten_properties_test.go +++ b/hack/generator/pkg/codegen/pipeline/flatten_properties_test.go @@ -30,9 +30,16 @@ func TestDuplicateNamesAreCaught(t *testing.T) { types.Add(astmodel.MakeTypeDefinition(astmodel.MakeTypeName(placeholderPackage, "objType"), objType)) result, err := applyPropertyFlattening(context.Background(), types) - // We don't fail but flattening does not occur + + // We don't fail but flattening does not occur, and flatten is set to false g.Expect(err).ToNot(HaveOccurred()) - g.Expect(result).To(Equal(types)) + + // identical to objType above but the "inner" prop is not set to flatten + newObjType := astmodel.NewObjectType().WithProperties(prop, innerObjProp.SetFlatten(false)) + expectedTypes := make(astmodel.Types) + expectedTypes.Add(astmodel.MakeTypeDefinition(astmodel.MakeTypeName(placeholderPackage, "objType"), newObjType)) + + g.Expect(result).To(Equal(expectedTypes)) } func TestFlatteningWorks(t *testing.T) { diff --git a/hack/generator/pkg/codegen/pipeline/status_augment.go b/hack/generator/pkg/codegen/pipeline/status_augment.go index 6888004ea93..23a4e8b56bb 100644 --- a/hack/generator/pkg/codegen/pipeline/status_augment.go +++ b/hack/generator/pkg/codegen/pipeline/status_augment.go @@ -7,6 +7,7 @@ package pipeline import ( "context" + "strings" "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" ) @@ -63,68 +64,189 @@ func fuseAugmenters(augments ...augmenter) augmenter { // flattenAugmenter copies across the "flatten" property from Swagger func flattenAugmenter(allTypes astmodel.ReadonlyTypes) augmenter { - return func(main astmodel.Type, swagger astmodel.Type) (astmodel.Type, error) { + /* TODO: there is a lot here that should be pulled into a "default augmenter" value that can be reused, + but at the moment we only have one augmenter */ + + return func(spec astmodel.Type, status astmodel.Type) (astmodel.Type, error) { + // reminder: merger is invoked with a pair of Types and invokes the first + // Added function that matches those types + // + // in this incarnation we are using it to handle the pair of values + // (SpecType, StatusType), to copy the StatusType’s “flatten” property + // across, if present (this is only provided on the Swagger = Status side) + // + // most of the cases are only recursively invoking the merger/matcher on + // “inner” types; the (Object, Object) case is where the work is done + // + // note that we do a small optimization where we don’t return a new type + // if nothing was changed; this allows us to keep the same TypeNames + // if no alterations were made + merger := astmodel.NewTypeMerger(func(ctx interface{}, left, right astmodel.Type) (astmodel.Type, error) { - // as a fallback, return left (= main) if we have nothing else to do + // as a fallback, return left (= main type) if we have nothing else to do return left, nil }) - merger.Add(func(main, swagger astmodel.TypeName) (astmodel.Type, error) { - // when we merge two typenames we know that (structurally) they must - // be the ‘same’ type, even if they have different names + // this is the main part of the merging, we want to match up (Object, Object) + // based on their properties and copy across “flatten” when present, and + // also invoke the merger on the (Spec, Status) types of each property recursively + merger.Add(func(spec, status *astmodel.ObjectType) (astmodel.Type, error) { + props := spec.Properties() + + changed := false + for ix, specProp := range props { + // find a matching property in the swagger spec + if swaggerProp, ok := status.Property(specProp.PropertyName()); ok { + // first copy over flatten property + if specProp.Flatten() != swaggerProp.Flatten() { + changed = true + specProp = specProp.SetFlatten(swaggerProp.Flatten()) + } + + // now recursively merge property types + newType, err := merger.Merge(specProp.PropertyType(), swaggerProp.PropertyType()) + if err != nil { + return nil, err + } + + if !newType.Equals(specProp.PropertyType()) { + changed = true + } + + props[ix] = specProp.WithType(newType) + } + } + + if !changed { + return spec, nil + } + + return spec.WithProperties(props...), nil + }) + + // handle (TypeName, Type) by resolving LHS + merger.Add(func(spec astmodel.TypeName, status astmodel.Type) (astmodel.Type, error) { + // Connascence alert! This is in coöperation with the code in + // determine_resource_ownership.go:updateChildResourceDefinitionsWithOwner + // which requires the typenames to identify which ChildResources match + // which Resources. The ChildResource types should never actually be + // used, so it's okay that we don't visit them here. + if strings.HasSuffix(spec.Name(), ChildResourceNameSuffix) { + // don't touch child resources! + return spec, nil + } - // this allows us to handle cases where names differ greatly from JSON schema to Swagger, - // we rely on the structure of the types to tell us which types are the same + specType := allTypes.Get(spec).Type() - newType, err := merger.Merge(allTypes.Get(main).Type(), allTypes.Get(swagger).Type()) + newSpec, err := merger.Merge(specType, status) if err != nil { return nil, err } - return newType, nil + // return original typename if not changed + if newSpec.Equals(specType) { + return spec, nil + } + + return newSpec, nil + }) + + // handle (Type, TypeName) by resolving RHS + merger.Add(func(spec astmodel.Type, status astmodel.TypeName) (astmodel.Type, error) { + return merger.Merge(spec, allTypes.Get(status).Type()) }) - // need to resolve main type - merger.Add(func(main astmodel.TypeName, swagger astmodel.Type) (astmodel.Type, error) { - newMain, err := merger.Merge(allTypes.Get(main).Type(), swagger) + // handle (Optional, Type) + merger.Add(func(spec *astmodel.OptionalType, status astmodel.Type) (astmodel.Type, error) { + // we ignore optionality when matching things up, since there are + // discordances between the JSON Schema/Swagger as some teams have handcrafted them + newSpecElement, err := merger.Merge(spec.Element(), status) if err != nil { return nil, err } - return newMain, nil + // return original type if not changed + if newSpecElement.Equals(spec.Element()) { + return spec, nil + } + + return astmodel.NewOptionalType(newSpecElement), nil + }) + + // handle (Type, Optional) + merger.Add(func(spec astmodel.Type, status *astmodel.OptionalType) (astmodel.Type, error) { + // we ignore optionality when matching things up, since there are + // discordances between the JSON Schema/Swagger as some teams have handcrafted them + return merger.Merge(spec, status.Element()) }) - // need to resolve swagger type - merger.Add(func(main astmodel.Type, swagger astmodel.TypeName) (astmodel.Type, error) { - result, err := merger.Merge(main, allTypes.Get(swagger).Type()) + // handle (FlaggedType, Type); there is no need to handle the opposite direction + // as the swagger types won’t be flagged + merger.Add(func(spec *astmodel.FlaggedType, status astmodel.Type) (astmodel.Type, error) { + newSpec, err := merger.Merge(spec.Element(), status) if err != nil { return nil, err } - return result, nil + // return original type if not changed + if newSpec.Equals(spec.Element()) { + return spec, nil + } + + return spec.WithElement(newSpec), nil }) - merger.Add(func(main, swagger *astmodel.ObjectType) (astmodel.Type, error) { - props := main.Properties() - for ix, mainProp := range props { - // find a matching property in the swagger spec - if swaggerProp, ok := swagger.Property(mainProp.PropertyName()); ok { - // first copy over flatten property - mainProp = mainProp.SetFlatten(swaggerProp.Flatten()) + // handle (Map, Map) by matching up the keys and values + merger.Add(func(spec, status *astmodel.MapType) (astmodel.Type, error) { + keyResult, err := merger.Merge(spec.KeyType(), status.KeyType()) + if err != nil { + return nil, err + } - // now recursively merge property types - newType, err := merger.Merge(mainProp.PropertyType(), swaggerProp.PropertyType()) - if err != nil { - return nil, err - } + valueResult, err := merger.Merge(spec.ValueType(), status.ValueType()) + if err != nil { + return nil, err + } - props[ix] = mainProp.WithType(newType) - } + // return original type if not changed + if keyResult.Equals(spec.KeyType()) && valueResult.Equals(spec.ValueType()) { + return spec, nil } - return main.WithProperties(props...), nil + return astmodel.NewMapType(keyResult, valueResult), nil + }) + + // handle (Array, Array) by matching up the inner types + merger.Add(func(spec, status *astmodel.ArrayType) (astmodel.Type, error) { + newSpecElement, err := merger.Merge(spec.Element(), status.Element()) + if err != nil { + return nil, err + } + + // return original type if not changed + if newSpecElement.Equals(spec.Element()) { + return spec, nil + } + + return astmodel.NewArrayType(newSpecElement), nil + }) + + // safety check, (AllOf, AllOf) should not occur + merger.Add(func(_, _ *astmodel.AllOfType) (astmodel.Type, error) { + panic("allofs should have been removed") + }) + + // safety check, (OneOf, OneOf) should not occur + merger.Add(func(_, _ *astmodel.OneOfType) (astmodel.Type, error) { + panic("oneofs should have been removed") + }) + + // this shouldn't ever happen, I think, (Resource, Resource) + merger.Add(func(_, _ *astmodel.ResourceType) (astmodel.Type, error) { + return astmodel.NewErroredType(nil, []string{"unsupported"}, nil), nil }) - return merger.Merge(main, swagger) + // now go! + return merger.Merge(spec, status) } }