Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

volumes: make Terraform import work #34

Merged
merged 8 commits into from
Feb 4, 2021
Merged

Conversation

okrause
Copy link
Contributor

@okrause okrause commented Dec 2, 2020

This PR is a possible solution for volume import (see issue #33 ).

It implements approach number 1. It is the most simply solution and should work well, but will fail for the very unlikely corner case that more than one volume hold a volume with the requested volume UUID.

A proper importer which can also handle cases where multiple volumes with the same ID are found would be the cleaner solution.

This is how to use it:

  1. Create an empty volume resource (volume.tf), e.g.:
resource "netapp-gcp_volume" "myvolume" {
}
  1. Lookup the volumeID of the volume you want to import in the UI, e.g 6462b343-262c-e785-dc6c-ca5fcfc7e185.
  2. Run the import
    terraform import netapp-gcp_volume.myvolume 6462b343-262c-e785-dc6c-ca5fcfc7e185
  3. If it worked fine, TF will have imported the state of the volume into the terraform.tfstate file. Use
    terraform show -no-color > volume.tf to create the resource definition (overwriting the prior empty definition).
  4. Delete the id line in volume.tf
  5. Run terraform plan. If everything went fine, it should run without proposing changes.
  6. Voila, you now have the definition in volume.tf and can start managing your volume with TF.

Happy to discuss.

@okrause
Copy link
Contributor Author

okrause commented Dec 2, 2020

Observation for shared-vpcs:
Importing and subsequent management of volumes with above method works fine.
Recreating the volume fails, since the TF files (generated via terraform show) are missing the shared_vpc_project_number attribute. It needs to be added manually to the TF file.

gcp/volume.go Outdated Show resolved Hide resolved
gcp/volume.go Outdated Show resolved Hide resolved
gcp/volume.go Outdated Show resolved Hide resolved
@okrause
Copy link
Contributor Author

okrause commented Dec 17, 2020

@lonico thank you for the input above.
Thinking about the ways to resolve multiple volumes with the same ID made me think about the actual problem again.

Our actual problem it, that ID is not guaranteed to be unique over all regions. The import command required a unique ID. So how would a unique ID look like? It is actually an (ID, region) tuple. Maybe we should simply require the user to specify both, e.g by requiring an ID in "CVSID:region_name" format. We could parse the components and don't need to guess the users intention.
Drawback is, that we cannot simply use the ID provided by the UI, might break user expectations and therefore need to document it well.

@lonico
Copy link
Contributor

lonico commented Dec 17, 2020

@okrause I like your idea about "CVSID:region_name". And we could support both format, if we assume that ":" cannot show in CSVID or region_name.
If we receive "CVSID", we try to find the region, but error out if multiple IDs are found. In the error message, we can ask for "CVSID:region_name" to resolve the ambiguity.
If we receive "CVSID:region_name" then we know where to look for.

@okrause
Copy link
Contributor Author

okrause commented Dec 18, 2020

@lonico Great proposal, like it a lot. It maintains user convenience and terraform import usage consistency, while proving a solution for this super rare corner case. Will update the PR over x-mas. Have a wonderful holiday.

- getAllVolumes is a helper function which returns all volumes in the
  project as a slice
- filterAllVolumes will apply a filter function to the list of all
  volumes and returns a slice of matching volumes
- getVolumeByID now uses filterAllVolumes in case region is not set
  (will happen for terraform import)
- getAllVolumes now takes region argument and is renamed to getVolumes
- refactored getVolumeByRegion and commented it out, since it is not
  used
support ID = <volumeID> and ID = <volumeID>:<region> for terraform
import
@okrause
Copy link
Contributor Author

okrause commented Dec 24, 2020

Hohohoho, here are coming the x-mas eve changes to terraform import.

A user can continue to import using the standard way (ID = <volumeID>), e.g.
terraform import netapp-gcp_volume.myvolume 1bc88bc6-cc7d-5fe3-3737-8e635fe2f996.
If only one volume with this ID exists, the import will succeed. If multiple volumes with the ID exist, an error message will instruct the user to use the alternative format ID = <volumeID>:<region>, e.g.
terraform import netapp-gcp_volume.myvolume 1bc88bc6-cc7d-5fe3-3737-8e635fe2f996:us-central1

I also did some refactoring to reduce redundant code.

Feedback welcome

When importing volumes on a shared VPC, we have to repopulate
attribute shared_vpc_project_number
Turns out API ((sometimes?)also returns long network path
without shared VPC. Make projectID detection more stable.
gcp/resource_netapp_gcp_volume.go Show resolved Hide resolved
gcp/volume.go Show resolved Hide resolved
gcp/volume.go Outdated Show resolved Hide resolved
gcp/volume.go Show resolved Hide resolved
@wenjun666 wenjun666 merged commit 347d04e into NetApp:master Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants