hetznercloud / terraform-provider-hcloud

Terraform Hetzner Cloud provider
https://registry.terraform.io/providers/hetznercloud/hcloud/latest
Mozilla Public License 2.0
495 stars 72 forks source link

Import fails to set image when snapshot is used #315

Closed sasoiliev closed 3 years ago

sasoiliev commented 3 years ago

Hi,

I'm logging this separately from #278 as I believe #278 is about using stock images that are no longer present.

This issue can be reproduced with version 1.24.1 of the provider like this:

Now terraform plan will report the server as a resource that will be re-created on apply.

Setting TF_LOG=INFO for the import command yields the following error:

2021-02-10T02:10:23.246+0100 [INFO]  plugin.terraform-provider-hcloud_v1.24.1: 2021/02/10 02:10:23 [ERROR] setting state: image: '' expected type 'string', got unconvertible type 'int': timestamp=2021-02-10T02:10:23.246+0100

The following diff seems to fix this:

diff --git a/internal/server/resource.go b/internal/server/resource.go
index 40cda8a2..85527567 100644
--- a/internal/server/resource.go
+++ b/internal/server/resource.go
@@ -635,7 +635,7 @@ func setServerSchema(d *schema.ResourceData, s *hcloud.Server) {
                if s.Image.Name != "" {
                        d.Set("image", s.Image.Name)
                } else {
-                       d.Set("image", s.Image.ID)
+                       d.Set("image", fmt.Sprintf("%d", s.Image.ID))
                }
        }
 }

However, not having much experience with Terraform provider testing, I didn't feel confident enough to implement an acceptance test for this use case, so I am refraining from opening a PR.

Thanks!

LKaemmerling commented 3 years ago

Hey @sasoiliev,

thank you for the report! I pushed an MR with your change proposal (which was correct!): https://github.com/hetznercloud/terraform-provider-hcloud/pull/316

Testing this specific case is a bit difficult because we can not create a snapshot from Terraform.

sasoiliev commented 3 years ago

Thanks @LKaemmerling!

Testing this specific case is a bit difficult because we can not create a snapshot from Terraform.

This is what stopped me from implementing a test myself as well. I was thinking that creating a snapshot with a certain label through the API client and adding a sweeper that sweeps the snapshot based on the label would work. If you think this makes sense I could give it a shot.

On a related note, I'm wondering why this error didn't propagate. Shouldn't this (trying to set a value of a resource property with an incorrect type) result in a failure of the top-level operation, i.e. a non-zero exit code of terraform import? I'm far from being a Terraform expert, but from an end-user perspective this seems to be the logical outcome.