Add source_file to incus_instance #172
Conversation
Signed-off-by: Lucas Bremgartner <[email protected]>
Signed-off-by: Lucas Bremgartner <[email protected]>
Signed-off-by: Lucas Bremgartner <[email protected]>
Signed-off-by: Lucas Bremgartner <[email protected]>
|
@maveonair I know, this PR is still in draft, but it works on my machine as well as in CI and therefore I would appreciate some feedback, especially if I have missed something. |
| @@ -795,6 +871,13 @@ func (r InstanceResource) SyncState(ctx context.Context, tfState *tfsdk.State, s | |||
| return respDiags | |||
| } | |||
|
|
|||
| if !m.SourceFile.IsNull() && !m.Devices.IsNull() { | |||
There was a problem hiding this comment.
But shouldn't be the devices already be created before and returned from the instance state? Why is this not the case in this scenario?
There was a problem hiding this comment.
My understanding is, that in this case, we do not actually define a device but we only use the configuration section to pass the name of the storage pool where the instance should be imported to.
This PR is about providing the same functionality to Terraform, that is provided by incus import. incus import does allow for a flag -storage, which is the name of the respective storage pool. This is what we provide in the property "pool"
The other properties that we provide to the device (namely: type = "disk" and path = "/") are basically ignored. We just have them to make the device entry look the same as a regular device entry would look like.
An alternative would have been to define source_file not as a string attribute but as a SingleNestedAttribute and then accept a property for the name of the backup file along side with a dedicated property for the storage pool. This has been discussed in #124.
@stgraber is this explanation correct? Can you provide more context?
|
|
||
| name := plan.Name.ValueString() | ||
|
|
||
| var poolName string |
There was a problem hiding this comment.
What happens when poolName is not set if len(plan.devices.Elements()) == 0?
There was a problem hiding this comment.
In this case, poolName is the zero value, which is the empty string.
Judging from https://github.com/lxc/incus/blob/21eee07711f872aaf6873f0a5f74094dcc5d85c0/client/incus_instances.go#L534, this is fine. If the extension container_backup_override_pool is present, this is handled automatically. If not, an error is returned from incus client.
| @@ -371,13 +384,73 @@ func (r InstanceResource) ValidateConfig(ctx context.Context, req resource.Valid | |||
| ) | |||
| } | |||
|
|
|||
| if !config.Image.IsNull() && !config.SourceInstance.IsNull() { | |||
| if !atMostOne(!config.Image.IsNull(), !config.SourceFile.IsNull(), !config.SourceInstance.IsNull()) { | |||
There was a problem hiding this comment.
I consider this harder to read than before, could we make this more readable?
There was a problem hiding this comment.
The reason for this is, that we now have 3 different options, from which at most one (0 or 1) may be used.
I see the following ways to change this code:
| if !atMostOne(!config.Image.IsNull(), !config.SourceFile.IsNull(), !config.SourceInstance.IsNull()) { | |
| if moreThanOne(!config.Image.IsNull(), !config.SourceFile.IsNull(), !config.SourceInstance.IsNull()) { |
This removes one negation !, which might make it a little bit more readable. On the other side, it is pretty common in Go to have negative condition to enter an error path (basically stating the condition positively and then negating it for error path).
| if !atMostOne(!config.Image.IsNull(), !config.SourceFile.IsNull(), !config.SourceInstance.IsNull()) { | |
| if maxOneEnabled(config.Image.IsNull(), config.SourceFile.IsNull(), config.SourceInstance.IsNull()) { |
This removes all the negations. The name could also be maxOneNonNull.
| if !atMostOne(!config.Image.IsNull(), !config.SourceFile.IsNull(), !config.SourceInstance.IsNull()) { | |
| if (!config.Image.IsNull() && (!config.SourceFile.IsNull() || !config.SourceInstance.IsNull())) || (!config.SourceFile.IsNull() && !config.SourceInstance.IsNull()) { |
As list of boolean conditions. Might work for now, but if we need to extend it to a fourth case, I expect this solution to become pretty unreadable.
Which of those do you prefer?
for source_file with storage Signed-off-by: Lucas Bremgartner <[email protected]>
Resolves: #124