Add external provider interface versioning #30
Conversation
execution/v0.1.1/commands.go
Outdated
|
|
||
| type ExecutionCommand string | ||
|
|
||
| const ( |
There was a problem hiding this comment.
This can be in a common package. If we decide to add more commands in newer versions of the interface, we can just use the new commands in the new version while the old version can ignore them.
execution/v0.1.1/execution.go
Outdated
|
|
||
| // If this is a CreateInstance command, we need to get the bootstrap params | ||
| // from stdin | ||
| if env.Command == CreateInstanceCommand { |
There was a problem hiding this comment.
There is a lot of common code here that is true for the old version and the new version. Dealing with stdin for example, the need to gather extra info for the CreateInstance command, etc.
We must see what actually differs between the old version and the new version, properly identify what we can reuse or make common and de-duplicate some of this code.
| // ExternalProvider defines an interface that external providers need to implement. | ||
| // This is very similar to the common.Provider interface, and was redefined here to | ||
| // decouple it, in case it may diverge from native providers. | ||
| type ExternalProvider interface { |
There was a problem hiding this comment.
Can we add a ValidatePoolInfo() function and a new associated command that receives image, flavor, provider config and extra specs and returns an error? This would allow us to run a validation check any time a pool is created or updated.
execution/v0.1.1/commands.go
Outdated
| StartInstanceCommand ExecutionCommand = "StartInstance" | ||
| StopInstanceCommand ExecutionCommand = "StopInstance" | ||
| RemoveAllInstancesCommand ExecutionCommand = "RemoveAllInstances" | ||
| ValidatePoolInfoCommand common.ExecutionCommand = "ValidatePoolInfo" |
There was a problem hiding this comment.
you can add these to the common package as well. Maybe in a separate const() block. This way you can avoid 2 imports in the v0.1.1 package.
Each package does a swirch anyway and only acts on particular commands. There is no overlap in command names, so feel free to move these in the common package as well.
Add a comment that they are a v0.1.1 set of commands and it should be fine.
| if e.ControllerID == "" { | ||
| return fmt.Errorf("missing controller ID") | ||
| } | ||
| case common.GetVersionCommand: |
There was a problem hiding this comment.
The GetVersion command currently manually baked into the existing providers will return the version of the provider itself:
There was a problem hiding this comment.
The interface version is already an exported field, which is parsed from the variables that the server sets, so there is little chance that the server will want to query that back. And a user will want to essentially get the version of the provider.
execution/v0.1.1/interface.go
Outdated
| // decouple it, in case it may diverge from native providers. | ||
| type ExternalProvider interface { | ||
| // CreateInstance creates a new compute instance in the provider. | ||
| CreateInstance(ctx context.Context, bootstrapParams params.BootstrapInstance) (params.ProviderInstance, error) |
There was a problem hiding this comment.
It seems that the signatures for the common functions both this version and the previous version, are identical. It makes sense to create a common interface with the common functions, and embed that common interface in both this one and the previous version.
If the previous version is identical to the base one, you can just create an alias to the base interface. Or just use the base interface directly.
There was a problem hiding this comment.
Here is an example of embedding an interface in another:
There was a problem hiding this comment.
I think it would be better to embed the interface than go directly for the common one. So I went for this implementation
This PR adds external provider interface versioning:
Interface Version,Pool IDandPool ExtraSpecs