Refactor to remove legacyconfig object #163
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -195,13 +195,26 @@ func constructComputeParameters(origSpec *runtime.RawExtension, iamRoleRef, iamI | |
| } | ||
|
|
||
| func New() *WiringPlugin { | ||
|
There was a problem hiding this comment. The members of this struct and it's use are the only changes to this class, everything else is just moving code from the extra package. |
||
| return &WiringPlugin{ | ||
| DeveloperRole: func(_ voyager.Location) []string { | ||
| return []string{"arn:aws:iam::123456789012:role/micros-server-iam-MicrosServer-ABC"} //example | ||
|
There was a problem hiding this comment. Legacy config is a major issue for manual or experimental testing of autowiring functions. You have to use the internal build to access this data. Tthe way shapes hold things together also practically requires all the functions be in this repo so you can find and reference them simply. There was a problem hiding this comment. I don't disagree, it would be nice to replace LegacyConfig with resource-specific configuration. There was a problem hiding this comment. There is a ticket to make autowiring functions accept configuration into their constructor. Even with legacy config completely gone we'd still need this functionality. There was a problem hiding this comment. That's exactly what I meant, if it wasn't clear :) There was a problem hiding this comment. We build the list of plugins. All of their "constructors" are accessible. There is not requirement to use known plugins object. There was a problem hiding this comment. Then the first trivial change would be to pass LegacyConfig to plugin constructors which use information from it, and remove it from the context? This would be a global LegacyConfig I suppose (not location-specific), but that's ok. It could also be a function providing a LegacyConfig for a given location. This should be a relatively simple isolated change, easy to review. There was a problem hiding this comment. Actually, we already have this function: https://github.com/atlassian/voyager/blob/master/pkg/orchestration/wiring/entangler.go#L161-L173 There was a problem hiding this comment. Look at the main function for orchestration. It takes registry.KnownWiringPlugins as an arg of type map[voyager.ResourceType]wiringplugin.WiringPlugin. There is no reason this needs to be the same map inside the close source repo. There was a problem hiding this comment. Also notice that master of this repo currently stubs legacy config with a nothing but empty strings. |
||
| }, | ||
| ManagedPolicies: func(_ voyager.Location) []string { | ||
| return []string{"arn:aws:iam::123456789012:policy/SOX-DENY-IAM-CREATE-DELETE", "arn:aws:iam::123456789012:policy/micros-iam-DefaultServicePolicy-ABC"} // example | ||
| }, | ||
| VPC: func(location voyager.Location) *oap.VPCEnvironment { | ||
| return oap.ExampleVPC(location.Label, location.Region) | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| type WiringPlugin struct { | ||
| DeveloperRole func(location voyager.Location) []string | ||
| ManagedPolicies func(location voyager.Location) []string | ||
| VPC func(location voyager.Location) *oap.VPCEnvironment | ||
| } | ||
|
|
||
| func (p *WiringPlugin) WireUp(stateResource *orch_v1.StateResource, context *wiringplugin.WiringContext) wiringplugin.WiringResult { | ||
| if stateResource.Type != ResourceType { | ||
| return &wiringplugin.WiringResultFailure{ | ||
| Error: errors.Errorf("invalid resource type: %q", stateResource.Type), | ||
|
|
@@ -221,7 +234,7 @@ func WireUp(stateResource *orch_v1.StateResource, context *wiringplugin.WiringCo | |
| } | ||
| } | ||
|
|
||
| return p.wireUp(userInput.Service.ID, ec2ComputePlanName, stateResource, context, constructComputeParameters) | ||
| } | ||
|
|
||
| func generateSecretResource(compute voyager.ResourceName, envVars map[string]string, dependencyReferences []smith_v1.Reference) (smith_v1.Resource, error) { | ||
|
|
@@ -264,7 +277,7 @@ func calculateServiceName(serviceName voyager.ServiceName, resourceName voyager. | |
| return microsServiceName, nil | ||
| } | ||
|
|
||
| func (p *WiringPlugin) wireUp(microServiceNameInSpec, ec2ComputePlanName string, stateResource *orch_v1.StateResource, context *wiringplugin.WiringContext, constructComputeParameters ConstructComputeParametersFunction) wiringplugin.WiringResult { | ||
| dependencies := context.Dependencies | ||
|
|
||
| if err := compute.ValidateASAPDependencies(context); err != nil { | ||
|
|
@@ -387,8 +400,8 @@ func wireUp(microServiceNameInSpec, ec2ComputePlanName string, stateResource *or | |
| bindingResources = append(bindingResources, secretResource) | ||
| } | ||
|
|
||
| assumeRoles := p.DeveloperRole(context.StateContext.Location) | ||
| managedPolicies := p.ManagedPolicies(context.StateContext.Location) | ||
|
|
||
| // The only things that generate IamSnippets are the things that have the correct shape | ||
| iamPluginInstanceSmithResource, err := iam.PluginServiceInstance( | ||
|
|
@@ -400,6 +413,7 @@ func wireUp(microServiceNameInSpec, ec2ComputePlanName string, stateResource *or | |
| context, | ||
| managedPolicies, | ||
| assumeRoles, | ||
| p.VPC(context.StateContext.Location), | ||
| ) | ||
| if err != nil { | ||
| return &wiringplugin.WiringResultFailure{ | ||
|
|
||
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.
The real problem, I think, is that autowiring functions at the moment are not configurable (they are all hardcoded), and the only way to pass configuration is via context and orchestration controller constructor
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.
What about moving the plugins out of wiring/registry? We can pass them as parameters into the constructor for orchestration controller.
This matches up with what @amckague has done for the example tag generator, and it's what we sort of do for smith too. We're going to want to split the wiring out of this repo anyway.