Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persist original ARM API version for use by generic controller #1606

Merged
merged 53 commits into from
Jul 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
f6c315e
Add error for pipeline stage occurring too early
theunrepentantgeek Jun 1, 2021
22fc434
Create OriginalGVK() function
theunrepentantgeek Jun 9, 2021
1403056
Code gardening
theunrepentantgeek Jun 9, 2021
9f663fd
Inject OriginalGVK functions and properties
theunrepentantgeek Jun 9, 2021
2cd02fe
Add OverlayWith() function for creating new Types sets
theunrepentantgeek Jun 9, 2021
7d85225
Include functions when collecting referenced types
theunrepentantgeek Jun 9, 2021
7724961
Fix test case generation for new types
theunrepentantgeek Jun 9, 2021
99468ba
Create OriginalVersion() function
theunrepentantgeek Jun 16, 2021
4e34f8b
Fix potential nil panic in ErroredType
theunrepentantgeek Jun 17, 2021
f46e3c5
Inject OriginalVersion function into Spec types
theunrepentantgeek Jun 23, 2021
45a312b
Convert Spec and Status types differently
theunrepentantgeek Jun 23, 2021
2b11996
Implement Equals() for ObjectTypeSerializationTestCase
theunrepentantgeek Jun 24, 2021
5e185e8
TypeWalker can handle external typenames
theunrepentantgeek Jun 24, 2021
f13a2b8
Add test for StorageTypeFactory
theunrepentantgeek Jun 25, 2021
ef9be7d
Fix formatting of composite literal
theunrepentantgeek Jun 26, 2021
65c0656
Code gardening
theunrepentantgeek Jun 26, 2021
a833c10
Simplify tests
theunrepentantgeek Jun 26, 2021
cc5e8a2
Add missing headers
theunrepentantgeek Jun 26, 2021
62e5105
Keep the CI pipeline happy.
theunrepentantgeek Jun 26, 2021
55e3f72
Collapse referenceTypes and outputTypes into one set
theunrepentantgeek Jun 29, 2021
a03b780
Code gardening
theunrepentantgeek Jun 29, 2021
e7ea0d8
Make builder explicit
theunrepentantgeek Jun 29, 2021
c77cab5
Split out GroupConversionGraph as a separate type and test
theunrepentantgeek Jun 29, 2021
3a676ba
Create ConversionGraph as a separate type and test
theunrepentantgeek Jun 29, 2021
a0f57c1
Factor out findSpecTypes() and findStatusTypes() for reuse
theunrepentantgeek Jun 29, 2021
fa74aea
Move factory methods into test package
theunrepentantgeek Jun 30, 2021
7c85731
Move injection of OriginalVersion() into dedicated pipeline stage
theunrepentantgeek Jun 30, 2021
67968a4
Move creation of StorageTypes into dedicated pipeline stage
theunrepentantgeek Jun 30, 2021
03fba1b
Move AsFunctionContainer() into astmodel
theunrepentantgeek Jun 30, 2021
25f00a0
Move AsPropertyContainer into astmodel
theunrepentantgeek Jun 30, 2021
c50f65c
Move InjectOriginalGVKFunction into dedicated pipeline stage
theunrepentantgeek Jun 30, 2021
9e3c5ac
Extract common building blocks for tests
theunrepentantgeek Jun 30, 2021
62b2eb5
Move InjectPropertyAssignmentFunctions to dedicated pipeline stage
theunrepentantgeek Jun 30, 2021
fbdf89e
Set up stage prerequisites
theunrepentantgeek Jun 30, 2021
4c7bdbf
Simplify storage type conversion
theunrepentantgeek Jun 30, 2021
98a323e
Integrate new stages into pipeline
theunrepentantgeek Jun 30, 2021
953f26a
Add pipeline stage InjectOriginalVersion property
theunrepentantgeek Jun 30, 2021
b3a4829
Update golden files
theunrepentantgeek Jun 30, 2021
46fe7d1
Fix Function receivers
theunrepentantgeek Jun 30, 2021
571c43d
Add missing headers
theunrepentantgeek Jun 30, 2021
02dc3f6
Fix tests
theunrepentantgeek Jun 30, 2021
b18c88d
Code gardening
theunrepentantgeek Jun 30, 2021
2ddebc4
Code gardening
theunrepentantgeek Jun 30, 2021
ed07fb3
go fmt
theunrepentantgeek Jul 5, 2021
e7bf505
Filter out ARM types
theunrepentantgeek Jul 5, 2021
6ecf285
Fix bug causing type conversion to be skipped
theunrepentantgeek Jul 5, 2021
2a9cb67
Empty commit
theunrepentantgeek Jul 5, 2021
74631c5
Code gardening
theunrepentantgeek Jul 7, 2021
4b4b717
Use intention revealing names for predicates
theunrepentantgeek Jul 7, 2021
f0eb8c5
Remove unused idFactory
theunrepentantgeek Jul 7, 2021
59d8dbc
Extract filter into predicate lambda local variable
theunrepentantgeek Jul 7, 2021
3be33df
Make it explicit that we inject OriginalVersion() before creating sto…
theunrepentantgeek Jul 7, 2021
393d2a7
Fixes for CI
theunrepentantgeek Jul 7, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions hack/generator/pkg/astbuilder/composite_literals.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* Copyright (c) Microsoft Corporation.
* Licensed under the MIT license.
*/

package astbuilder

import (
"github.com/dave/dst"
)

// CompositeLiteralDetails captures the information required to generate code for an inline struct initialization
type CompositeLiteralDetails struct {
structType dst.Expr
elts []dst.Expr
}

// NewCompositeLiteralDetails creates a new instance for initialization of the specified struct
// structType is an expression to handle both structs from the current package and imported ones requiring qualification
func NewCompositeLiteralDetails(structType dst.Expr) *CompositeLiteralDetails {
return &CompositeLiteralDetails{
structType: structType,
}
}

// AddField adds initialization of another field
// Returns the receiver to allow method chaining when desired
func (details *CompositeLiteralDetails) AddField(name string, value dst.Expr) {
expr := &dst.KeyValueExpr{
Key: dst.NewIdent(name),
Value: dst.Clone(value).(dst.Expr),
}

expr.Decs.Before = dst.NewLine
expr.Decs.After = dst.NewLine

details.elts = append(details.elts, expr)
}

// Build constructs the actual dst.CompositeLit that's required
func (details CompositeLiteralDetails) Build() *dst.CompositeLit {
return &dst.CompositeLit{
Type: details.structType,
Elts: details.elts,
}
}
6 changes: 5 additions & 1 deletion hack/generator/pkg/astmodel/errored_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,11 @@ func (e *ErroredType) Unwrap() Type {
// types is a dictionary for resolving named types
func (e *ErroredType) WriteDebugDescription(builder *strings.Builder, types Types) {
builder.WriteString("Error[")
e.inner.WriteDebugDescription(builder, types)
if e.inner != nil {
e.inner.WriteDebugDescription(builder, types)
} else {
builder.WriteString("<missing>")
}

for _, e := range e.errors {
builder.WriteString("|")
Expand Down
18 changes: 17 additions & 1 deletion hack/generator/pkg/astmodel/function_container.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,25 @@
package astmodel

// FunctionContainer is implemented by Types that contain functions
// Can't include the withers for modification until we have generics
// Provides readonly access as we need to use a TypeVisitor for modifications to preserve type wrapping
type FunctionContainer interface {
// Functions returns all the function implementations
// A sorted slice is returned to preserve immutability and provide determinism
Functions() []Function

// HasFunctionWithName determines if this resource has a function with the given name
HasFunctionWithName(name string) bool
}

// AsFunctionContainer converts a type into a function container
// Only use this readonly access as we must use a TypeVisitor for modifications to preserve type wrapping
func AsFunctionContainer(theType Type) (FunctionContainer, bool) {
theunrepentantgeek marked this conversation as resolved.
Show resolved Hide resolved
switch t := theType.(type) {
case FunctionContainer:
return t, true
case MetaType:
return AsFunctionContainer(t.Unwrap())
default:
return nil, false
}
}
5 changes: 4 additions & 1 deletion hack/generator/pkg/astmodel/object_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,10 @@ func (objectType *ObjectType) References() TypeNameSet {
results.AddAll(property.PropertyType().References())
}

// Not collecting types from functions deliberately.
matthchr marked this conversation as resolved.
Show resolved Hide resolved
for _, fn := range objectType.functions {
results.AddAll(fn.References())
}

return results
}

Expand Down
15 changes: 14 additions & 1 deletion hack/generator/pkg/astmodel/property_container.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
package astmodel

// PropertyContainer is implemented by Types that contain properties
// Can't include the withers for modification until we have generics
// Provides readonly access as we need to use a TypeVisitor for modifications to preserve type wrapping
type PropertyContainer interface {
// Properties returns all the properties from this container
// A sorted slice is returned to preserve immutability and provide determinism
Expand All @@ -15,3 +15,16 @@ type PropertyContainer interface {
// Property returns the property and true if the named property is found, nil and false otherwise
Property(name PropertyName) (*PropertyDefinition, bool)
}

// AsPropertyContainer converts a type into a property container
// Only use this readonly access as we must use a TypeVisitor for modifications to preserve type wrapping
func AsPropertyContainer(theType Type) (PropertyContainer, bool) {
theunrepentantgeek marked this conversation as resolved.
Show resolved Hide resolved
switch t := theType.(type) {
case PropertyContainer:
return t, true
case MetaType:
return AsPropertyContainer(t.Unwrap())
default:
return nil, false
}
}
6 changes: 6 additions & 0 deletions hack/generator/pkg/astmodel/resource_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,12 @@ func (resource *ResourceType) Functions() []Function {
return functions
}

// HasFunctionWithName determines if this resource has a function with the given name
func (resource *ResourceType) HasFunctionWithName(name string) bool {
_, ok := resource.functions[name]
return ok
}

// References returns the types referenced by Status or Spec parts of the resource
func (resource *ResourceType) References() TypeNameSet {
spec := resource.spec.References()
Expand Down
23 changes: 18 additions & 5 deletions hack/generator/pkg/astmodel/std_references.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,13 @@ var (
APIExtensionsJSONReference = MakeExternalPackageReference("k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/JSON")
APIMachineryErrorsReference = MakeExternalPackageReference("k8s.io/apimachinery/pkg/util/errors")
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")
APIMachinerySchemaReference = MakeExternalPackageReference("k8s.io/apimachinery/pkg/runtime/schema")

ClientGoSchemeReference = MakeExternalPackageReference("k8s.io/client-go/kubernetes/scheme")
ControllerRuntimeAdmission = MakeExternalPackageReference("sigs.k8s.io/controller-runtime/pkg/webhook/admission")
ControllerRuntimeConversion = MakeExternalPackageReference("sigs.k8s.io/controller-runtime/pkg/conversion")
ControllerSchemeReference = MakeExternalPackageReference("sigs.k8s.io/controller-runtime/pkg/scheme")
GitHubErrorsReference = MakeExternalPackageReference("github.com/pkg/errors")

// References to libraries used for testing
CmpReference = MakeExternalPackageReference("github.com/google/go-cmp/cmp")
Expand All @@ -43,8 +47,17 @@ var (
// Imports with specified names
GomegaImport = NewPackageImport(GomegaReference).WithName(".")

// Type names
// Type names - GenRuntime
ResourceReferenceTypeName = MakeTypeName(GenRuntimeReference, "ResourceReference")
KnownResourceReferenceTypeName = MakeTypeName(GenRuntimeReference, "KnownResourceReference")
JSONTypeName = MakeTypeName(APIExtensionsReference, "JSON")
ToARMConverterInterfaceType = MakeTypeName(GenRuntimeReference, "ToARMConverter")

// Type names - API Machinery
GroupVersionKindTypeName = MakeTypeName(APIMachinerySchemaReference, "GroupVersionKind")
SchemeType = MakeTypeName(APIMachineryRuntimeReference, "Scheme")
JSONTypeName = MakeTypeName(APIExtensionsReference, "JSON")

// Type names - Controller Runtime
ConvertibleInterface = MakeTypeName(ControllerRuntimeConversion, "Convertible")
HubInterface = MakeTypeName(ControllerRuntimeConversion, "Hub")
)
1 change: 1 addition & 0 deletions hack/generator/pkg/astmodel/type_walker.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ func (t *TypeWalker) visitTypeName(this *TypeVisitor, it TypeName, ctx interface
if err != nil {
return nil, errors.Wrapf(err, "visitTypeName failed for name %q", it)
}

it, ok := visitedTypeName.(TypeName)
if !ok {
panic(fmt.Sprintf("TypeWalker visitor visitTypeName must return a TypeName, instead returned %T", visitedTypeName))
Expand Down
2 changes: 1 addition & 1 deletion hack/generator/pkg/astmodel/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (types Types) FullyResolve(t Type) (Type, error) {
}

// AddAll adds multiple definitions to the set, with the same safety check as Add() to panic if a duplicate is included
func (types Types) AddAll(otherDefinitions []TypeDefinition) {
func (types Types) AddAll(otherDefinitions ...TypeDefinition) {
for _, t := range otherDefinitions {
types.Add(t)
}
Expand Down
8 changes: 2 additions & 6 deletions hack/generator/pkg/astmodel/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,7 @@ func Test_TypesAddAll_GivenTypes_ModifiesSet(t *testing.T) {
g := NewGomegaWithT(t)

types := createTestTypes(alphaDefinition, betaDefinition)
otherTypes := []TypeDefinition{gammaDefinition, deltaDefinition}

types.AddAll(otherTypes)
types.AddAll(gammaDefinition, deltaDefinition)

g.Expect(types).To(ContainElements(gammaDefinition, deltaDefinition))
}
Expand All @@ -61,9 +59,7 @@ func Test_TypesAddAll_GivenOverlappingTypes_Panics(t *testing.T) {
g := NewGomegaWithT(t)

types := createTestTypes(alphaDefinition, betaDefinition)
otherTypes := []TypeDefinition{betaDefinition, deltaDefinition}

g.Expect(func() { types.AddAll(otherTypes) }).To(Panic())
g.Expect(func() { types.AddAll(betaDefinition, deltaDefinition) }).To(Panic())
}

/*
Expand Down
20 changes: 18 additions & 2 deletions hack/generator/pkg/codegen/code_generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (

"github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel"
"github.com/Azure/azure-service-operator/hack/generator/pkg/codegen/pipeline"
"github.com/Azure/azure-service-operator/hack/generator/pkg/codegen/storage"
"github.com/Azure/azure-service-operator/hack/generator/pkg/config"
)

Expand Down Expand Up @@ -78,6 +79,10 @@ 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()

return []pipeline.Stage{

pipeline.LoadSchemaIntoTypes(idFactory, configuration, pipeline.DefaultSchemaLoader),
Expand Down Expand Up @@ -151,7 +156,14 @@ func createAllPipelineStages(idFactory astmodel.IdentifierFactory, configuration
pipeline.AddCrossplaneEmbeddedResourceSpec(idFactory).UsedFor(pipeline.CrossplaneTarget),
pipeline.AddCrossplaneEmbeddedResourceStatus(idFactory).UsedFor(pipeline.CrossplaneTarget),

pipeline.CreateStorageTypes(idFactory).UsedFor(pipeline.ARMTarget), // TODO: For now only used for ARM
// Create Storage types
//TODO: For now only used for ARM
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra mega minor:

Suggested change
//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),
pipeline.InjectPropertyAssignmentFunctions(graph, idFactory).UsedFor(pipeline.ARMTarget),
pipeline.InjectOriginalGVKFunction(idFactory).UsedFor(pipeline.ARMTarget),

pipeline.SimplifyDefinitions(),
pipeline.InjectJsonSerializationTests(idFactory).UsedFor(pipeline.ARMTarget),

Expand Down Expand Up @@ -218,7 +230,11 @@ func (generator *CodeGenerator) verifyPipeline() error {
}

for _, postreq := range stage.Postrequisites() {
stagesExpected[postreq] = append(stagesExpected[postreq], stage.Id())
if _, ok := stagesSeen[postreq]; ok {
errs = append(errs, errors.Errorf("postrequisite %q of stage %q satisfied too early", postreq, stage.Id()))
} else {
stagesExpected[postreq] = append(stagesExpected[postreq], stage.Id())
}
}

stagesSeen[stage.Id()] = struct{}{}
Expand Down
10 changes: 9 additions & 1 deletion hack/generator/pkg/codegen/golden_files_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,15 @@ func NewTestCodeGenerator(testName string, path string, t *testing.T, testConfig
// TODO: This isn't as clean as would be liked -- should we remove panic from RemoveStages?
switch genPipeline {
case config.GenerationPipelineAzure:
codegen.RemoveStages("deleteGenerated", "rogueCheck", "createStorage", "reportTypesAndVersions")
codegen.RemoveStages(
"deleteGenerated",
"rogueCheck",
"createStorageTypes",
"injectOriginalGVKFunction",
"injectOriginalVersionFunction",
"injectOriginalVersionProperty",
"injectPropertyAssignmentFunctions",
"reportTypesAndVersions")
if !testConfig.HasARMResources {
codegen.RemoveStages("createArmTypes", "applyArmConversionInterface")
// These stages treat the collection of types as a graph of types rooted by a resource type.
Expand Down
73 changes: 30 additions & 43 deletions hack/generator/pkg/codegen/pipeline/create_storage_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,69 +8,56 @@ package pipeline
import (
"context"

kerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/klog/v2"
"github.com/pkg/errors"

"github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel"
"github.com/Azure/azure-service-operator/hack/generator/pkg/codegen/storage"
)

const CreateStorageTypesStageId = "createStorageTypes"

// 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(idFactory astmodel.IdentifierFactory) Stage {
return MakeStage(
"createStorage",
func CreateStorageTypes(conversionGraph *storage.ConversionGraph) Stage {
result := MakeStage(
CreateStorageTypesStageId,
"Create storage versions of CRD types",
func(ctx context.Context, types astmodel.Types) (astmodel.Types, error) {

// Create a factory for each group (aka service) and divvy up the types
factories := make(map[string]*storage.StorageTypeFactory)
for name, def := range types {
// Predicate to isolate both resources and complex objects
isPropertyContainer := func(def astmodel.TypeDefinition) bool {
_, ok := astmodel.AsPropertyContainer(def.Type())
return ok
}

ref, ok := name.PackageReference.AsLocalPackage()
if !ok {
// Skip definitions from non-local packages
// (should never happen)
klog.Warningf("Skipping storage type generation for unexpected non-local package reference %q", name.PackageReference)
continue
}
// Predicate to filter out ARM types
isNotARMType := func(def astmodel.TypeDefinition) bool {
return !astmodel.ARMFlag.IsOn(def.Type())
}

factory, ok := factories[ref.Group()]
if !ok {
klog.V(3).Infof("Creating storage factory for %s", ref.Group())
factory = storage.NewStorageTypeFactory(ref.Group(), idFactory)
factories[ref.Group()] = factory
}
// Filter to the types we want to process
typesToConvert := types.Where(isPropertyContainer).Where(isNotARMType)

if astmodel.ARMFlag.IsOn(def.Type()) {
// skip ARM types as they don't need storage variants
continue
}
storageTypes := make(astmodel.Types)
typeConverter := storage.NewTypeConverter(types)

factory.Add(def)
}

// Collect up all the results
result := make(astmodel.Types)
var errs []error
for _, factory := range factories {
t, err := factory.Types()
// Create storage variants
for name, def := range typesToConvert {
storageDef, err := typeConverter.ConvertDefinition(def)
if err != nil {
errs = append(errs, err)
continue
return nil, errors.Wrapf(err, "creating storage variant of %q", name)
}

result.AddTypes(t)
storageTypes.Add(storageDef)
conversionGraph.AddLink(name.PackageReference, storageDef.Name().PackageReference)
matthchr marked this conversation as resolved.
Show resolved Hide resolved
}

err := kerrors.NewAggregate(errs)
if err != nil {
return nil, err
}

unmodified := types.Except(result)
result.AddTypes(unmodified)
result := types.Copy()
result.AddTypes(storageTypes)
return result, nil
})

result.RequiresPrerequisiteStages(injectOriginalVersionFunctionStageId)
return result
}
Loading