From 7bfe46083a81bba7b7e369d5ff3975adfa0a6c1c Mon Sep 17 00:00:00 2001 From: Matthew Christopher Date: Fri, 18 Jun 2021 16:17:29 -0700 Subject: [PATCH] Add new pipeline stage that merges groups - In the case that group A references group B, we need to pull group B's types into group A. - Fixes #1578 --- hack/generator/pkg/codegen/code_generator.go | 3 + .../pkg/codegen/golden_files_test.go | 9 ++- .../pipeline_collapse_cross_group_refs.go | 57 +++++++++++++++++ .../testdata/ARMCodeGeneratorPipeline.golden | 1 + ...version_on_arm_type_only_crossplane.golden | 5 -- ...t_resource_and_ownership_crossplane.golden | 61 +++++++++++++++++-- ...empty_objecttype_removed_crossplane.golden | 6 -- ...embedded_resource_inside_crossplane.golden | 7 --- ...source_multiple_contexts_crossplane.golden | 5 -- ...mbedded_resource_removed_crossplane.golden | 6 -- ...est_embedded_subresource_crossplane.golden | 6 -- ...resource_same_properties_crossplane.golden | 6 -- ...st_id_resource_reference_crossplane.golden | 5 -- ...onal_resource_references_crossplane.golden | 5 -- ...esource_array_properties_crossplane.golden | 5 -- ...ource_complex_properties_crossplane.golden | 5 -- ...ple_resource_json_fields_crossplane.golden | 5 -- ..._resource_map_properties_crossplane.golden | 5 -- ...le_resource_renders_spec_crossplane.golden | 5 -- 19 files changed, 122 insertions(+), 85 deletions(-) create mode 100644 hack/generator/pkg/codegen/pipeline_collapse_cross_group_refs.go diff --git a/hack/generator/pkg/codegen/code_generator.go b/hack/generator/pkg/codegen/code_generator.go index bdad6ebfedd..3f8c809b780 100644 --- a/hack/generator/pkg/codegen/code_generator.go +++ b/hack/generator/pkg/codegen/code_generator.go @@ -119,6 +119,9 @@ func createAllPipelineStages(idFactory astmodel.IdentifierFactory, configuration // Strip out redundant type aliases: removeTypeAliases(), + // Collapse cross group references + collapseCrossGroupReferences(), + // De-pluralize resource types // (Must come after type aliases are resolved) improveResourcePluralization(). diff --git a/hack/generator/pkg/codegen/golden_files_test.go b/hack/generator/pkg/codegen/golden_files_test.go index d2b419741c7..223b0146ebf 100644 --- a/hack/generator/pkg/codegen/golden_files_test.go +++ b/hack/generator/pkg/codegen/golden_files_test.go @@ -151,17 +151,20 @@ func NewTestCodeGenerator(testName string, path string, t *testing.T, testConfig codegen.RemoveStages("deleteGenerated", "rogueCheck", "createStorage", "reportTypesAndVersions") if !testConfig.HasARMResources { codegen.RemoveStages("createArmTypes", "applyArmConversionInterface") - // removeEmbeddedResources treats the collection of types as a graph of types rooted by a resource type. + // These stages treat the collection of types as a graph of types rooted by a resource type. // In the degenerate case where there are no resources it behaves the same as stripUnreferenced - removing // all types. Remove it in phases that have no resources to avoid this. - codegen.RemoveStages("removeEmbeddedResources") + codegen.RemoveStages("removeEmbeddedResources", "collapseCrossGroupReferences") + codegen.ReplaceStage("stripUnreferenced", stripUnusedTypesPipelineStage()) } else { codegen.ReplaceStage("addCrossResourceReferences", addCrossResourceReferencesForTest(idFactory)) } case config.GenerationPipelineCrossplane: codegen.RemoveStages("deleteGenerated", "rogueCheck") - codegen.ReplaceStage("stripUnreferenced", stripUnusedTypesPipelineStage()) + if !testConfig.HasARMResources { + codegen.ReplaceStage("stripUnreferenced", stripUnusedTypesPipelineStage()) + } default: return nil, errors.Errorf("unknown pipeline kind %q", string(pipeline)) diff --git a/hack/generator/pkg/codegen/pipeline_collapse_cross_group_refs.go b/hack/generator/pkg/codegen/pipeline_collapse_cross_group_refs.go new file mode 100644 index 00000000000..9db587777b4 --- /dev/null +++ b/hack/generator/pkg/codegen/pipeline_collapse_cross_group_refs.go @@ -0,0 +1,57 @@ +/* + * Copyright (c) Microsoft Corporation. + * Licensed under the MIT license. + */ + +package codegen + +import ( + "context" + + "github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" + "github.com/pkg/errors" +) + +// collapseCrossGroupReferences finds and removes references between API groups. This isn't particularly common +// but does occur in a few instances, for example from Microsoft.Compute -> Microsoft.Compute.Extensions. +func collapseCrossGroupReferences() PipelineStage { + return MakePipelineStage( + "collapseCrossGroupReferences", + "Finds and removes cross group references", + func(ctx context.Context, types astmodel.Types) (astmodel.Types, error) { + resources := astmodel.CollectResourceDefinitions(types) + result := make(astmodel.Types) + + for resourceName := range resources { + walker := newTypeWalker(types, resourceName) + updatedTypes, err := walker.Walk(types[resourceName]) + if err != nil { + return nil, errors.Wrapf(err, "failed walking types") + } + + for _, newDef := range updatedTypes { + err := result.AddAllowDuplicates(newDef) + if err != nil { + return nil, err + } + } + } + + return result, nil + }) +} + +func newTypeWalker(types astmodel.Types, resourceName astmodel.TypeName) *astmodel.TypeWalker { + visitor := astmodel.TypeVisitorBuilder{}.Build() + walker := astmodel.NewTypeWalker(types, visitor) + walker.AfterVisit = func(original astmodel.TypeDefinition, updated astmodel.TypeDefinition, ctx interface{}) (astmodel.TypeDefinition, error) { + if !resourceName.PackageReference.Equals(updated.Name().PackageReference) { + // Note: If we ever find this generating colliding names, we might need to introduce a unique suffix. + // For now though it doesn't seem to, so preserving the shorter names as they're clearer. + updated = updated.WithName(astmodel.MakeTypeName(resourceName.PackageReference, updated.Name().Name())) + } + return astmodel.IdentityAfterVisit(original, updated, ctx) + } + + return walker +} diff --git a/hack/generator/pkg/codegen/testdata/ARMCodeGeneratorPipeline.golden b/hack/generator/pkg/codegen/testdata/ARMCodeGeneratorPipeline.golden index c95a3c109db..0f3f79529b1 100644 --- a/hack/generator/pkg/codegen/testdata/ARMCodeGeneratorPipeline.golden +++ b/hack/generator/pkg/codegen/testdata/ARMCodeGeneratorPipeline.golden @@ -9,6 +9,7 @@ nameTypes Name inner types for CRD propertyRewrites Applying type transformers to properties determineResourceOwnership Determine ARM resource relationships removeAliases Remove type aliases +collapseCrossGroupReferences Finds and removes cross group references pluralizeNames Improve resource pluralization stripUnreferenced Strip unreferenced types assertTypesStructureValid Asserts that the types collection is valid diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_oneof_resource_conversion_on_arm_type_only_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_oneof_resource_conversion_on_arm_type_only_crossplane.golden index 2d97f118a7c..93a0e3bc9f1 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_oneof_resource_conversion_on_arm_type_only_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_oneof_resource_conversion_on_arm_type_only_crossplane.golden @@ -28,11 +28,6 @@ type FakeResourceList struct { Items []FakeResource `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - Test *FakeResource `json:"test,omitempty"` -} - type FakeResource_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider FakeResourceParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_dependent_resource_and_ownership_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_dependent_resource_and_ownership_crossplane.golden index ae7275d60e0..b8670289a91 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_dependent_resource_and_ownership_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_dependent_resource_and_ownership_crossplane.golden @@ -68,11 +68,24 @@ type CList struct { Items []C `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - A *A `json:"a,omitempty"` - B *B `json:"b,omitempty"` - C *C `json:"c,omitempty"` +// +kubebuilder:rbac:groups=test.infra.azure.com,resources=ds,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=test.infra.azure.com,resources={ds/status,ds/finalizers},verbs=get;update;patch + +// +kubebuilder:object:root=true +// +kubebuilder:storageversion +//Generated from: https://test.test/schemas/2020-01-01/test.json#/resourceDefinitions/D +type D struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + Spec D_Spec `json:"spec,omitempty"` +} + +// +kubebuilder:object:root=true +//Generated from: https://test.test/schemas/2020-01-01/test.json#/resourceDefinitions/D +type DList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitempty"` + Items []D `json:"items"` } type A_Spec struct { @@ -90,6 +103,11 @@ type C_Spec struct { ForProvider CParameters `json:"forProvider"` } +type D_Spec struct { + v1alpha1.ResourceSpec `json:",inline"` + ForProvider DParameters `json:"forProvider"` +} + type AParameters struct { // +kubebuilder:validation:Required APIVersion ASpecAPIVersion `json:"apiVersion"` @@ -143,6 +161,27 @@ type CParameters struct { Type CSpecType `json:"type"` } +type DParameters struct { + AName string `json:"aName"` + ANameRef *v1alpha1.Reference `json:"aNameRef,omitempty"` + ANameSelector *v1alpha1.Selector `json:"aNameSelector,omitempty"` + + // +kubebuilder:validation:Required + APIVersion DSpecAPIVersion `json:"apiVersion"` + BName string `json:"bName"` + BNameRef *v1alpha1.Reference `json:"bNameRef,omitempty"` + BNameSelector *v1alpha1.Selector `json:"bNameSelector,omitempty"` + + // +kubebuilder:validation:Required + Name string `json:"name"` + ResourceGroupName string `json:"resourceGroupName"` + ResourceGroupNameRef *v1alpha1.Reference `json:"resourceGroupNameRef,omitempty"` + ResourceGroupNameSelector *v1alpha1.Selector `json:"resourceGroupNameSelector,omitempty"` + + // +kubebuilder:validation:Required + Type DSpecType `json:"type"` +} + // +kubebuilder:validation:Enum={"2020-06-01"} type ASpecAPIVersion string @@ -173,6 +212,16 @@ type CSpecType string const CSpecTypeMicrosoftAzureC = CSpecType("Microsoft.Azure/C") +// +kubebuilder:validation:Enum={"2020-06-01"} +type DSpecAPIVersion string + +const DSpecAPIVersion20200601 = DSpecAPIVersion("2020-06-01") + +// +kubebuilder:validation:Enum={"Microsoft.Azure/D"} +type DSpecType string + +const DSpecTypeMicrosoftAzureD = DSpecType("Microsoft.Azure/D") + func init() { - SchemeBuilder.Register(&A{}, &AList{}, &B{}, &BList{}, &C{}, &CList{}) + SchemeBuilder.Register(&A{}, &AList{}, &B{}, &BList{}, &C{}, &CList{}, &D{}, &DList{}) } diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_empty_objecttype_removed_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_empty_objecttype_removed_crossplane.golden index f2b9737bb3f..5208a5a8bf9 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_empty_objecttype_removed_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_empty_objecttype_removed_crossplane.golden @@ -48,12 +48,6 @@ type BList struct { Items []B `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - A *A `json:"a,omitempty"` - B *B `json:"b,omitempty"` -} - type A_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider AParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_has_embedded_resource_inside_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_has_embedded_resource_inside_crossplane.golden index fec7f45c83f..c3f493fae62 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_has_embedded_resource_inside_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_has_embedded_resource_inside_crossplane.golden @@ -68,13 +68,6 @@ type CList struct { Items []C `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - A *A `json:"a,omitempty"` - B *B `json:"b,omitempty"` - C *C `json:"c,omitempty"` -} - type A_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider AParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_multiple_contexts_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_multiple_contexts_crossplane.golden index 7b43969a883..cb22e82f8c2 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_multiple_contexts_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_multiple_contexts_crossplane.golden @@ -28,11 +28,6 @@ type AList struct { Items []A `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - A *A `json:"a,omitempty"` -} - type A_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider AParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_removed_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_removed_crossplane.golden index 10104878678..e2ab30e3a7b 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_removed_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_resource_removed_crossplane.golden @@ -48,12 +48,6 @@ type BList struct { Items []B `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - A *A `json:"a,omitempty"` - B *B `json:"b,omitempty"` -} - type A_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider AParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_subresource_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_subresource_crossplane.golden index 0e004d7289f..1430dd44dca 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_subresource_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_subresource_crossplane.golden @@ -48,12 +48,6 @@ type BList struct { Items []B `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - A *A `json:"a,omitempty"` - B *B `json:"b,omitempty"` -} - type A_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider AParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_subresource_same_properties_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_subresource_same_properties_crossplane.golden index 2bae6e28bdd..f6bfccf4c16 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_subresource_same_properties_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_embedded_subresource_same_properties_crossplane.golden @@ -48,12 +48,6 @@ type BList struct { Items []B `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - A *A `json:"a,omitempty"` - B *B `json:"b,omitempty"` -} - type A_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider AParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_id_resource_reference_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_id_resource_reference_crossplane.golden index 17ff6021d21..91367d164a3 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_id_resource_reference_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_id_resource_reference_crossplane.golden @@ -28,11 +28,6 @@ type FakeResourceList struct { Items []FakeResource `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - Test *FakeResource `json:"test,omitempty"` -} - type FakeResource_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider FakeResourceParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_required_and_optional_resource_references_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_required_and_optional_resource_references_crossplane.golden index cca796cefc6..9d0b7bc1ad8 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_required_and_optional_resource_references_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_required_and_optional_resource_references_crossplane.golden @@ -28,11 +28,6 @@ type FakeResourceList struct { Items []FakeResource `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - Test *FakeResource `json:"test,omitempty"` -} - type FakeResource_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider FakeResourceParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_array_properties_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_array_properties_crossplane.golden index 7e5974db1be..7c8b780a88c 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_array_properties_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_array_properties_crossplane.golden @@ -28,11 +28,6 @@ type FakeResourceList struct { Items []FakeResource `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - Test *FakeResource `json:"test,omitempty"` -} - type FakeResource_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider FakeResourceParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_complex_properties_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_complex_properties_crossplane.golden index 6fcd9b20bc2..6af05d61c15 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_complex_properties_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_complex_properties_crossplane.golden @@ -28,11 +28,6 @@ type FakeResourceList struct { Items []FakeResource `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - Test *FakeResource `json:"test,omitempty"` -} - type FakeResource_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider FakeResourceParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_json_fields_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_json_fields_crossplane.golden index f8feca186f8..73de36907f2 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_json_fields_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_json_fields_crossplane.golden @@ -29,11 +29,6 @@ type FakeResourceList struct { Items []FakeResource `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - Test *FakeResource `json:"test,omitempty"` -} - type FakeResource_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider FakeResourceParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_map_properties_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_map_properties_crossplane.golden index e73d0f613ac..2d1c51662dd 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_map_properties_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_map_properties_crossplane.golden @@ -28,11 +28,6 @@ type FakeResourceList struct { Items []FakeResource `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - Test *FakeResource `json:"test,omitempty"` -} - type FakeResource_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider FakeResourceParameters `json:"forProvider"` diff --git a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_renders_spec_crossplane.golden b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_renders_spec_crossplane.golden index bfd82070a04..1b55bc717aa 100644 --- a/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_renders_spec_crossplane.golden +++ b/hack/generator/pkg/codegen/testdata/ArmResource/Arm_test_simple_resource_renders_spec_crossplane.golden @@ -28,11 +28,6 @@ type FakeResourceList struct { Items []FakeResource `json:"items"` } -//Generated from: https://test.test/schemas/2020-01-01/test.json -type Test struct { - Test *FakeResource `json:"test,omitempty"` -} - type FakeResource_Spec struct { v1alpha1.ResourceSpec `json:",inline"` ForProvider FakeResourceParameters `json:"forProvider"`