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

Add ability to lock VPS #35

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

JoooostB
Copy link
Contributor

@JoooostB JoooostB commented Sep 7, 2020

As of right now the provider has no support for setting the Customer Lock for a VPS (only for reading). In our workflow we like to set the Customer Lock directly after ordering, as we've had some issues with the front-end before where the wrong VPS got modified, upgraded or even deleted.

When ordering a VPS, TransIP's API does not support setting a Customer Lock on the VPS directly. However, when updating the VPS after you've ordered it, you'll be able to set the Lock. Hence why I added the Update function and changed line 236 to Update instead of Read the VPS, so the lock will be set if configured; even when the VPS was not created yet.

If you've got any feedback or suggestions, please let me know.

@JoooostB JoooostB changed the title Lock VPS from Terraform Add ability to lock VPS Sep 7, 2020
Copy link
Owner

@aequitas aequitas left a comment

Choose a reason for hiding this comment

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

The feature itself is welcome, but I'm hesitant to enable Update on this resource without a full implementation and tests, see comments on the source.

resource_transip_vps.go Outdated Show resolved Hide resolved
@@ -17,7 +18,7 @@ func resourceVps() *schema.Resource {
return &schema.Resource{
Create: resourceVpsCreate,
Read: resourceVpsRead,
// Update: resourceVpsUpdate,
Update: resourceVpsUpdate,
Copy link
Owner

Choose a reason for hiding this comment

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

By allowing updates we give the expectation to the user all (non-computed) attributes on this resource can be updated. This will be reflected in the plan as well. But the implementation below does not take care of all these situations.

We should either:

  1. implement Update fully
  2. perform the customer lock update at the end of the create step.

I'm ok with either solution, but the second one seems the quickest to get this feature included for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've worked on implementing the update fully, but had some trouble adding tags to a VPS. I commented it out for now, but for your information it resulted in the following error:

panic: interface conversion: interface {} is []interface {}, not []string

resource_transip_vps.go Outdated Show resolved Hide resolved
@aequitas
Copy link
Owner

I've tested this locally but the VPS does not seem to unlock with the update.

=== RUN   TestAccTransipResourceVpsUpdate
    TestAccTransipResourceVpsUpdate: testing.go:654: Step 1 error: Check failed: Check 1/2 error: transip_vps.test: Attribute 'is_customer_locked' expected "false", got "true"
    TestAccTransipResourceVpsUpdate: testing.go:715: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: errors during apply: failed to cancel VPS "aequitasterraformtest-vps16": VPS 'aequitasterraformtest-vps16' is customer locked, no modification is allowed
        
        State: transip_vps.test:
          ID = aequitasterraformtest-vps16
          provider = provider.transip
          availability_zone = ams0
          cpus = 1
          description = test-1600676840-updated
          disk_size = 52428800
          install_text = 
          ip_address = 37.97.129.162
          ipv4_addresses.# = 1
          ipv4_addresses.0 = 37.97.129.162
          ipv6_addresses.# = 1
          ipv6_addresses.0 = 2a01:7c8:aabc:5ff::1
          is_blocked = false
          is_customer_locked = true
          mac_address = 52:54:00:1b:2c:2e
          memory_size = 1048576
          name = aequitasterraformtest-vps16
          operating_system = ubuntu-20.04
          product_name = vps-bladevps-x1
          status = running
          tags.# = 0
--- FAIL: TestAccTransipResourceVpsUpdate (81.69s)
FAIL
exit status 1
FAIL	github.com/aequitas/terraform-provider-transip	81.713s

@JoooostB JoooostB marked this pull request as draft March 26, 2021 17:03
@JoooostB JoooostB marked this pull request as ready for review March 31, 2021 18:08
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