Removed Factory from CloudProvider #460
Conversation
cmd/controller/main.go
Outdated
| @@ -73,7 +73,7 @@ func main() { | |||
| }) | |||
|
|
|||
| clientSet := kubernetes.NewForConfigOrDie(manager.GetConfig()) | |||
| cloudProvider := registry.NewCloudProvider(cloudprovider.Options{ClientSet: clientSet}) | |||
| cloudProvider := registry.NewAPI(cloudprovider.Options{ClientSet: clientSet}) | |||
There was a problem hiding this comment.
IMO keep this as register.NewCloudProvider. I actually renamed a bunch of factories to just cloudprovider in an earlier PR to make it easier for you.
pkg/cloudprovider/aws/api.go
Outdated
| } | ||
| ) | ||
|
|
||
| type API struct { |
There was a problem hiding this comment.
IMO you should keep this concept named "cloud provider" since it's so common across the k8s ecosystem. Maybe cloudprovider.go?
pkg/cloudprovider/aws/node.go
Outdated
| @@ -26,12 +26,12 @@ import ( | |||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |||
| ) | |||
|
|
|||
| type NodeFactory struct { | |||
| type NodeAPI struct { | |||
There was a problem hiding this comment.
I'm not sold on this naming change. This isn't an API, it's a https://en.wikipedia.org/wiki/Factory_method_pattern. Any time you see .For() in my code, it's a factory. Factories are hugely useful when necessary. In this case, the NodeFactory encapsulates some dependencies (e.g. EC2) vends node objects.
There was a problem hiding this comment.
I agree. Good point. I'll change this back.
pkg/cloudprovider/aws/validation.go
Outdated
| @@ -25,11 +25,11 @@ import ( | |||
| ) | |||
|
|
|||
| // Validate cloud provider specific components of the cluster spec | |||
| func (c *Capacity) Validate(ctx context.Context) (errs *apis.FieldError) { | |||
| func (a *API) Validate(ctx context.Context, spec *v1alpha1.ProvisionerSpec) (errs *apis.FieldError) { | |||
There was a problem hiding this comment.
Can we move these functions aws/cloudprovider.go (name change recommended above). It's only a few lines, and I think the scope is easier to encapsulate now. It feels a bit awkward that this is standing alone now that the cloudprovider API surface is so much smaller (Create, Delete, Validate, GetInstanceTypes)
pkg/cloudprovider/fake/api.go
Outdated
| NotImplementedError = fmt.Errorf("provider is not implemented. Are you running the correct release for your cloud provider?") | ||
| ) | ||
|
|
||
| type API struct { |
There was a problem hiding this comment.
I wouldn't make this name change for similar reasons. This looks like a bad merge to me. I removed the NotImplementedError (which isn't used anymore) in a previous PR.
pkg/cloudprovider/fake/api.go
Outdated
| return &API{ | ||
| WantErr: NotImplementedError, | ||
| NodeReplicas: make(map[string]*int32), | ||
| NodeGroupStable: true, |
There was a problem hiding this comment.
Similarly, I removed this. It seems like a bad merge.
pkg/cloudprovider/registry/README.md
Outdated
| @@ -8,8 +8,8 @@ import ( | |||
| "github.com/awslabs/karpenter/pkg/cloudprovider/<YOUR_PROVIDER_NAME>" | |||
| ) | |||
|
|
|||
| func NewFactory() cloudprovider.Factory { | |||
| return <YOUR_PROVIDER_NAME>.NewFactory() | |||
| func NewAPI() cloudprovider.API { | |||
pkg/cloudprovider/types.go
Outdated
| // Create a set of nodes for each of the given constraints. | ||
| Create(context.Context, []*Packing) ([]*PackedNode, error) | ||
| Create(context.Context, []*Packing, *v1alpha1.Provisioner) ([]*PackedNode, error) |
There was a problem hiding this comment.
Pass the provisioner first, imo.
There was a problem hiding this comment.
Before Packing, but not before context, right?
| @@ -44,7 +44,7 @@ func TestAPIs(t *testing.T) { | |||
|
|
|||
| var controller *Controller | |||
| var env = test.NewEnvironment(func(e *test.Environment) { | |||
| cloudProvider := &fake.Factory{} | |||
| cloudProvider := fake.NewAPI() | |||
There was a problem hiding this comment.
I this change is caused by the bad merge.
Issue #, if available:
#441
Description of changes:
Removed concept of Factories from our codebase. This moves away from the programming concept of RAII that we used to make API calls to the cloud provider. We no longer need a factory to instantiate the API for us on a call.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.