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

feat: delete ISO even if already attached #131

Merged

Conversation

johnrichardrinehart
Copy link
Contributor

Description

It's currently impossible to change the URL of an ISO resource that's attached to an instance. This PR allows for the deletion of an ISO that's attached to an instance at deletion-time.

Code/Logical Structure

  1. Try do delete the ISO
  2. If deletion errors, then we check to see if the error message contains information about attachment to an instance (only the IP address, the error message doesn't contain an instance ID)
  3. If we detect an IP address of an instance then we try to associated the IP address with the instance ID
  4. If we determine the instance ID then we try to detach the ISO from the instance (waiting until completion)
  5. If detachment is successful, then we recursively call resourceVultrIsoDelete to attempt to delete again.

Related Issues

#128

Checklist:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you linted your code locally prior to submission?
  • Have you successfully ran tests with your changes locally? Yes, but I have written no unit tests. I tested a few times against my terraform infrastructure.

@ddymko
Copy link
Contributor

ddymko commented Apr 22, 2021

@johnrichardrinehart Thanks for submitting this - I'll try to to get this by end of the week if not I'll have a review for ya by monday

@johnrichardrinehart
Copy link
Contributor Author

@ddymko Absolutely no rush. Thanks for the ping.

@ddymko ddymko self-requested a review April 26, 2021 22:48
refactor: use single loop for instance association with ISO Id
refactor: made error messages more consistent
fix: return from resourceVultrIsoDelete as soon as ISO is detached and
deleted successfully
Copy link
Contributor

@ddymko ddymko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally looks good

@ddymko
Copy link
Contributor

ddymko commented Apr 28, 2021

@johnrichardrinehart Thanks for taking the time to implement this. We have some other features we want to release along side this. We are going to tag this in a 2.3.0 milestone and plan to release next week

@ddymko ddymko merged commit 06d229b into vultr:master Apr 28, 2021
@johnrichardrinehart
Copy link
Contributor Author

@ddymko Thanks for taking the time to work with me on a review. I'm much happier since we made the changes you suggested. I still don't like my solution for detecting if it's still attached (https://github.com/vultr/terraform-provider-vultr/pull/131/files#diff-9440d6827d830f384c9118b4fc75b2aea56b1c826f826fc780dbef8c208831f5R122). But, without error constants/values it's not going to look great.

I'm excited for the release and I'll be using this feature myself. Hopefully it benefits someone else :) .

@johnrichardrinehart johnrichardrinehart deleted the john/fix-iso-attach-error-message branch April 28, 2021 15:09
@ddymko ddymko mentioned this pull request May 11, 2021
3 tasks
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.

2 participants