tailscale: add import logic for remaining resources #445
Conversation
Add import logic for `posture_integration` and `logstream_configuration` and corresponding acceptance tests. Add acceptance tests to exercise import logic that was recently added for `device_key`, `device_subnet_routes`, `dns_nameservers`, and `dns_search_paths`. Updates #441 Signed-off-by: Mario Minardi <mario@tailscale.com>
mpminardi
force-pushed
the
mpminardi/remaining-import
branch
from
411ac4a to
c2486b8
Compare
| // TODO(mpminardi): investigate changing the ID in state to be the device_id instead | ||
| // in an eventual major version bump. | ||
| d.Set("device_id", d.Id()) | ||
| d.SetId(createUUID()) |
There was a problem hiding this comment.
Interesting.
What were the issues with just using plain device id? I didn't experience any problems with importing the subnet just by passthrough.
This logic seems a bit unnecessary but maybe I'm not understanding something in this context.
The UUID creation is also present in the other device resources as well, so I'm not following why it has to be UUID for this particular resource but not the other device resources.
I mean I get why it has to be UUID when creating the resource, but when importing it can straight be the device_id, nothing assumes this has to be UUID tbqh.
Anyway, still works, was just curious to see this changed for only device_subnet, and not for device_key, device_tags, device_authorization.
Maybe it needs to change for those as well?
@mpminardi
EDIT: I think the other resources I broke backward compatibility with read, you probably need to change device_key, device_tags, device_authorization in a similar manner.
There was a problem hiding this comment.
Hey @reegnz!
The rest of the device* resources are using device_id as the id in state upon resource creation (and were before the import changes), so in those cases the passthrough can be used and works as expected (device_key, device_tags, and device_authorization for reference).
For this resource the change here would end up breaking existing users of the resource as the Id of existing device_subnet resources are a UUID in state, meaning that we would try and fail to fetch the device using the UUID instead of the device Id for non-imported resources.
This is why we need to do the more complex import logic in this case: so that we can properly read both when doing the import and when doing a read operation for an existing resource. Doing a fresh import would not have surfaced this issue as the read would be using the passthrough Id (e.g., the device_id) and work properly for that scenario.
This resource should also use the device_id as its id, but since we have released versions where it uses a UUID instead we need to keep this in UUID format to stay within what we can release without doing a major version revision as per the HashiCorp versioning best practices.
Apologies as well for not catching this on the original review! I had forgotten that this resource was using a UUID for its Id and completely missed that this would cause issues originally.
Add import logic for
posture_integrationandlogstream_configurationand corresponding acceptance tests.Add acceptance tests to exercise import logic that was recently added for
device_key,device_subnet_routes,dns_nameservers, anddns_search_paths.Updates #441