-
Notifications
You must be signed in to change notification settings - Fork 204
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
Implement the Convertible interface on non-hub resources #1628
Changes from all commits
a978fae
0883b1f
472f91b
7073068
5082aec
179728a
82cf36b
66d41b2
cc7ade0
3d0587e
fe69aba
3fb22f1
aceb5b2
fa8ac7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/* | ||
* Copyright (c) Microsoft Corporation. | ||
* Licensed under the MIT license. | ||
*/ | ||
|
||
package astmodel | ||
|
||
// InterfaceInjector is a utility for injecting interface implementations into resources and objects | ||
type InterfaceInjector struct { | ||
// visitor is used to do the actual injection | ||
visitor TypeVisitor | ||
} | ||
|
||
// NewInterfaceInjector creates a new interface injector for modifying resources and objects | ||
func NewInterfaceInjector() *InterfaceInjector { | ||
result := &InterfaceInjector{} | ||
|
||
result.visitor = TypeVisitorBuilder{ | ||
VisitObjectType: result.injectInterfaceIntoObject, | ||
VisitResourceType: result.injectInterfaceIntoResource, | ||
}.Build() | ||
|
||
return result | ||
} | ||
|
||
// Inject modifies the passed type definition by injecting the passed function | ||
func (i *InterfaceInjector) Inject(def TypeDefinition, implementation *InterfaceImplementation) (TypeDefinition, error) { | ||
result, err := i.visitor.VisitDefinition(def, implementation) | ||
if err != nil { | ||
return TypeDefinition{}, err | ||
} | ||
return result, nil | ||
} | ||
|
||
// injectFunctionIntoObject takes the function provided as a context and includes it on the | ||
// provided object type | ||
func (i *InterfaceInjector) injectInterfaceIntoObject( | ||
_ *TypeVisitor, ot *ObjectType, ctx interface{}) (Type, error) { | ||
implementation := ctx.(*InterfaceImplementation) | ||
return ot.WithInterface(implementation), nil | ||
} | ||
|
||
// injectFunctionIntoResource takes the function provided as a context and includes it on the | ||
// provided resource type | ||
func (i *InterfaceInjector) injectInterfaceIntoResource( | ||
_ *TypeVisitor, rt *ResourceType, ctx interface{}) (Type, error) { | ||
fn := ctx.(*InterfaceImplementation) | ||
return rt.WithInterface(fn), nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
/* | ||
* Copyright (c) Microsoft Corporation. | ||
* Licensed under the MIT license. | ||
*/ | ||
|
||
package pipeline | ||
|
||
import ( | ||
"context" | ||
|
||
"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" | ||
"github.com/Azure/azure-service-operator/hack/generator/pkg/functions" | ||
) | ||
|
||
// ImplementConvertibleInterfaceStageId is the unique identifier for this pipeline stage | ||
const ImplementConvertibleInterfaceStageId = "implementConvertibleInterface" | ||
|
||
// ImplementConvertibleInterface injects the functions ConvertTo() and ConvertFrom() into each non-hub Resource | ||
// Type, providing the required implementation of the Convertible interface needed by the controller | ||
func ImplementConvertibleInterface(idFactory astmodel.IdentifierFactory) Stage { | ||
|
||
stage := MakeStage( | ||
ImplementConvertibleInterfaceStageId, | ||
"Implement the Convertible interface on each non-hub Resource type", | ||
func(ctx context.Context, state *State) (*State, error) { | ||
injector := astmodel.NewInterfaceInjector() | ||
|
||
modifiedTypes := make(astmodel.Types) | ||
resources := storage.FindResourceTypes(state.Types()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this method (and the others in Reading this made me think it was finding the storage resource types or something, I had to go look at the impl to understand it was just any old resource type. Your There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I'm sure moved these into There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you consider pulling it forward? I've got at least one upcoming PR where I'd like to use the Status one. |
||
for name, def := range resources { | ||
resource, ok := astmodel.AsResourceType(def.Type()) | ||
if !ok { | ||
// Skip non-resources (though, they should be filtered out, above) | ||
continue | ||
} | ||
|
||
if resource.IsStorageVersion() { | ||
// The hub storage version doesn't implement Convertible | ||
continue | ||
} | ||
|
||
convertible := createConvertibleInterfaceImplementation( | ||
name, resource, state.ConversionGraph(), idFactory) | ||
if convertible.FunctionCount() > 0 { | ||
modified, err := injector.Inject(def, convertible) | ||
if err != nil { | ||
return nil, errors.Wrapf(err, "injecting Convertible interface into %s", name) | ||
} | ||
|
||
modifiedTypes.Add(modified) | ||
} | ||
} | ||
|
||
newTypes := state.Types().OverlayWith(modifiedTypes) | ||
return state.WithTypes(newTypes), nil | ||
}) | ||
|
||
stage.RequiresPrerequisiteStages(InjectPropertyAssignmentFunctionsStageId) | ||
return stage | ||
} | ||
|
||
// createConvertibleInterfaceImplementation creates the required implementation of conversion.Convertible, ready for | ||
// injection onto the resource. The ConvertTo() and ConvertFrom() methods chain the required conversion between resource | ||
// versions, but are dependent upon previously injected AssignPropertiesTo() and AssignPropertiesFrom() methods to | ||
// actually copy information across. See resource_conversion_function.go for more information. | ||
func createConvertibleInterfaceImplementation( | ||
name astmodel.TypeName, | ||
resource *astmodel.ResourceType, | ||
conversionGraph *storage.ConversionGraph, | ||
idFactory astmodel.IdentifierFactory) *astmodel.InterfaceImplementation { | ||
var conversionFunctions []astmodel.Function | ||
|
||
for _, fn := range resource.Functions() { | ||
if propertyAssignmentFn, ok := fn.(*functions.PropertyAssignmentFunction); ok { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A comment explaining what the relationship between the PropertyAssignmentFunctions and and Convertible might be useful here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it expected that there are resources without a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hub resources don't have property assignment functions - conversions are always to/from types closer to the hub; hub resources have nothing to convert with. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but this function isn't ever invoked with hub resources anyway, because of the
in the loop yes? That's why I was wondering if you wanted to tolerate types without those functions or if it made sense to just fail fast if you found one since that meant something suspicious was going on. |
||
hub := conversionGraph.FindHubTypeName(name) | ||
conversionFn := functions.NewResourceConversionFunction(hub, propertyAssignmentFn, idFactory) | ||
conversionFunctions = append(conversionFunctions, conversionFn) | ||
} | ||
} | ||
|
||
return astmodel.NewInterfaceImplementation(astmodel.ConvertibleInterface, conversionFunctions...) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
/* | ||
* Copyright (c) Microsoft Corporation. | ||
* Licensed under the MIT license. | ||
*/ | ||
|
||
package pipeline | ||
|
||
import ( | ||
"context" | ||
"testing" | ||
|
||
. "github.com/onsi/gomega" | ||
|
||
"github.com/Azure/azure-service-operator/hack/generator/pkg/astmodel" | ||
"github.com/Azure/azure-service-operator/hack/generator/pkg/test" | ||
) | ||
|
||
// TestInjectConvertibleInterface checks that the pipeline stage does what we expect when run in relative isolation, | ||
// with only a few expected (and closely reated) stages in operation | ||
func TestInjectConvertibleInterface(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this test be replaced with a standard If so, consider leaving a TODO here to that effect? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the purpose of this test is to test this pipeline stage in isolation (well, as much isolation as possible). I don't want to conflate the results of this test with other things happening to independent stages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment. |
||
g := NewGomegaWithT(t) | ||
|
||
idFactory := astmodel.NewIdentifierFactory() | ||
// Test Resource V1 | ||
|
||
specV1 := test.CreateSpec(test.Pkg2020, "Person", test.FullNameProperty) | ||
statusV1 := test.CreateStatus(test.Pkg2020, "Person") | ||
resourceV1 := test.CreateResource(test.Pkg2020, "Person", specV1, statusV1) | ||
|
||
// Test Resource V2 | ||
|
||
specV2 := test.CreateSpec(test.Pkg2021, "Person", test.FullNameProperty) | ||
statusV2 := test.CreateStatus(test.Pkg2021, "Person") | ||
resourceV2 := test.CreateResource(test.Pkg2021, "Person", specV2, statusV2) | ||
|
||
types := make(astmodel.Types) | ||
types.AddAll(resourceV1, specV1, statusV1, resourceV2, specV2, statusV2) | ||
|
||
state := NewState().WithTypes(types) | ||
|
||
// Run CreateConversionGraph first to populate the conversion graph | ||
createConversionGraph := CreateConversionGraph() | ||
state, err := createConversionGraph.Run(context.TODO(), state) | ||
g.Expect(err).To(Succeed()) | ||
|
||
// Run CreateStorageTypes to create additional resources into which we will inject | ||
createStorageTypes := CreateStorageTypes() | ||
state, err = createStorageTypes.Run(context.TODO(), state) | ||
g.Expect(err).To(Succeed()) | ||
|
||
// Run InjectPropertyAssignmentFunctions to create those functions | ||
injectPropertyFns := InjectPropertyAssignmentFunctions(idFactory) | ||
state, err = injectPropertyFns.Run(context.TODO(), state) | ||
g.Expect(err).To(Succeed()) | ||
|
||
// Now run our stage | ||
injectFunctions := ImplementConvertibleInterface(idFactory) | ||
state, err = injectFunctions.Run(context.TODO(), state) | ||
g.Expect(err).To(Succeed()) | ||
|
||
// When verifying the golden file, check that the implementations of ConvertTo() and ConvertFrom() are what you | ||
// expect - if you don't have expectations, check that they do the right thing. | ||
test.AssertPackagesGenerateExpectedCode(t, state.Types(), t.Name()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make more sense to just expose this as
Functions()
like exists onResourceType
? (Can we do that since this is also embedded there?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functions returns a copy (by necessity), whereas this can return the count directly. Not that we're too performance sensitive, but that's why I added. it. 😁