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

[BUG] Update AttachVolume method to use VolumeAttachConfig due to civogo v0.3.75 upgrade #340

Open
Praveen005 opened this issue Sep 3, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@Praveen005
Copy link
Contributor

Description

What's the Issue?


When we update the Provider's civogo dependency to v0.3.75, which the latest one,

the AttachVolume method expects us to provide VolumeAttachConfig instead of instanceID.

_, err := apiClient.AttachVolume(volumeID, instanceID)

Type VolumeAttachConfig was added recently, as evident in the screenshot below:

image


Steps to reproduce:


Just update the dependency using go get github.com/civo/civogo command.

Acceptance Criteria

  • Should be able to create the resource_volume_attachment with the updated dependency
@Praveen005 Praveen005 added the bug Something isn't working label Sep 3, 2024
@Praveen005
Copy link
Contributor Author

Hi @uzaxirr

I worked on this and would like explain my approach for your review.

I stumbled upon this while working on #332. Comment Here highlights that.

Earlier AttachVolume method expected an instance ID to be passed along with volume ID, but now requires
VolumeAttachConfig to be passed in place of instanceID.

https://github.com/civo/civogo/blob/4fee779533b0cfd41ff2f53b4ea57b052848ac05/volume.go#L44-L49

Fix:

image
image

volumeConfig.Region = apiClient.Region sets the Region to the value specified in provider config.

image

up next,

if region, ok := d.GetOk("region"); ok {
	apiClient.Region = region.(string)
	volumeConfig.Region = region.(string)
}

overwrites the Region, if it is specified in the resource config.

Specifying Region is necessary because, volume attachment will fail if there is a region mismatch.

More context:

I faced a peculiar issue, my default region was NYC1 but provider had LON1 and I didn't set Region field of the volumeConfig, it took the default NYC1 while other resources had LON1 and I was presented with a completely unrelated error.

Screenshot 2024-09-04 001745

Setting volumeConfig.Region fixed this.

Other changes are trivial, like setting the volumeConfig.InstanceID and then passing volumeConfig to the AttachVolume method.


Result:

Successful resource_volume_attachment creation:

Screenshot 2024-09-04 000610 ------------------------------------------------------------------------------

Questions:

Should the sample code in the doc here be updated to include civo_firewall resource, since it is a required field for civo_instance resource?

I have also changed civo_instances_size to civo_size in the doc.

Thank You!

@uzaxirr
Copy link
Member

uzaxirr commented Sep 5, 2024

Feel free to raise a PR

@Praveen005
Copy link
Contributor Author

Hi @uzaxirr,

#341 would fix this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants