search

equinix / terraform-equinix-metal-anthos-on-vsphere

[Deprecated] Automated Anthos Installation via Terraform for Equinix Metal with vSphere
https://registry.terraform.io/modules/equinix/anthos-on-vsphere/metal/latest
Apache License 2.0
63 stars 41 forks source link

remove error check on govc datastore.mkdir #112

Closed dfong closed 2 months ago

dfong commented 3 years ago

when testing with anthos 1.5, i got an error from the mkdir apparently because the directory already existed.

so i propose removing the error check (which i had added in #88) from this command.

dfong commented 3 years ago

Meaning it works with 1.6, 1.5, & 1.4 ?

anthos 1.4: worked with or without the change.

anthos 1.5: does not work with or without the change. with this change, it gets farther, though.

anthos 1.6: did not try yet.

in short, i am unable to test on versions beyond 1.4.

displague commented 3 years ago

@dfong do we understand how gke-on-prem/ already exists?

I would rather skip this step if it is not needed. Does this only occur when re-applying or does this error happen the initial provision?

dfong commented 3 years ago

do we understand how gke-on-prem/ already exists?

i am not sure how it happens. i would note, however, that it was my PR #88 that added this check. before that, none of the commands in this script were being checked for error exit. any errors that occurred would have been ignored. so i think removing the check on ONE of the commands won't make things any worse than they were before my PR was merged.

it is quite possible that i got the error when re-running the script manually on the admin-workstation after it failed and was reported by terraform.

as a general principle, these scripts should be idempotent. ie, when a failure occurs at some point in the terraform apply, ideally it "should be" possible to re-run the script. if the script fails when the directory already exists, then it won't be idempotent.

displague commented 3 years ago

@dfong I'm just looking at this again following the project rename and additions of integration testing (although failing, they are present).

It looks like we can pass -p to the datastore.mkdir https://github.com/vmware/govmomi/blob/master/govc/USAGE.md#datastoremkdir, assuming this works like mkdir -p it should not complain on existing directories.