Upgrade DNSimple provider to API v2 #10760

weppos · 2016-12-15T15:33:46Z

A couple of days ago DNSimple announced the next major release of the API. Among the various improvements, API v2 supports the following new features:

OAuth
Multi-accounts

In the last months, few users contacted us in particular about the second point, as they created a separate account (e.g. for the company) and they wanted to be able to use it via Terraform.

This PR upgrades the current DNSimple provider to API v2, and replaces @pearkes's Go client with the official DNSimple Go client.

I tried to follow the instructions in the CONTRIBUTING.md file, but I'd be happy if someone can review the changes and provide a feedback. Some notes:

I removed @pearkes's client from the vendor folder, and added the DNSimple one in a separate commit, as requested in the CONTRIBUTING file.
I upgraded the provider to v2
I needed to access some config inside the resources, hence I added a Client wrapper (similar approaches were adopted e.g. in the GitHub provider and others) in ba723c5. Please let me know if there is a preferred way.

Open questions:

API v1 and API v2 are not compatible. The API token is different, as well the auth mechanism. I've tried to add a warning if the user had the previous token configured. Is there a better way?
Some names in the resource no longer really reflect the API structure. For e.g, we replaced "domain" with "zone", that it will be more relevant also when we will expose reverse zones. Any objection to change them as part of this major change?
There are certain resource fields that Terraform defines as String, although they are actually integers (e.g. TTL, Priority). I've seen a very limited usage of non-String types, even in other providers. Is there a recommended approach? Should I leave them String?

The documentation is not updated yet, I'll work on it, including feedback from this PR (if any). I will probably highlight API v2 support. Does it make sense to provide an API v1 to v2 Terraform upgrade section?

stack72 · 2016-12-15T15:39:48Z

Hey @weppos

Thanks for this work - this is a great addition to the provider - I would love to talk about this work more (upgrade path etc) - please can you drop me an email? stack72 at hashicorp.com

Thanks

P.

weppos · 2016-12-15T15:49:33Z

@stack72 👍 done, email sent!

weppos · 2017-01-28T05:40:11Z

Hi @stack72, do you have any update about this PR and/or the email?

weppos · 2017-02-07T12:50:33Z

Sorry to bother again, but I was wondering it there's any way I can help to move forward with the review of this PR.

As we have a fixed deadline for API v1 removal, it would be amazing if I can get this completed in advance so that users will be able to take their time to migrate. :)

weppos · 2017-02-27T17:07:25Z

❤️

stack72 · 2017-02-27T18:06:53Z

Hey @weppos

I promise i will get back to you on this tomorrow - are you EU based? If so, could we have a discussion on this? I can se up a quick hangout for tomorrow to discuss

Paul

weppos · 2017-02-27T18:19:08Z

Yes, I am in GMT+1. I'd be happy to jump in an hangout. Feel free to reply to the email I sent with the details :)

stack72 · 2017-02-27T18:20:16Z

Perfect! Doing so now :)

Acceptance tests pass: ``` === RUN TestProvider --- PASS: TestProvider (0.00s) === RUN TestProvider_impl --- PASS: TestProvider_impl (0.00s) === RUN TestAccDNSimpleRecord_Basic --- PASS: TestAccDNSimpleRecord_Basic (2.67s) === RUN TestAccDNSimpleRecord_Updated --- PASS: TestAccDNSimpleRecord_Updated (1.88s) PASS ok github.com/hashicorp/terraform/builtin/providers/dnsimple ``` Note that the code still has to be updated to pass the account ID dynamically in place of "TODO-ACCOUNT".

The config is required as the new API wants to know the identifier of the account you are operating to. The account is not stored in the client (as the client can talk with different accounts), hence I need to pass it as part of the config.

weppos · 2017-03-01T16:20:03Z

I generally don't rebase against master, but in this case the merge caused 4k files to be changed, and the diff was +925,296 −86,913.

I decided to rebase instead.

Provide upgrade instructions and update the docs for API v2.

weppos · 2017-03-01T18:16:30Z

@stack72 the PR is now ready for review (and merge).

I updated the docs and the integration, but I haven't added the schema deprecations. I am still reading other providers to get more details, and determine an action plan.

To avoid adding too many changes here, I would probably open a fresh PR with deprecations, if any is required.

If any of you is using this provider, I'd love to get feedbacks. I haven't been able to try it directly (except for running the test suites). I'll try to create a Terraform local setup to play with it.

stack72 · 2017-03-02T05:12:25Z

Hi @weppos

Serious love for this :) LGTM and the tests pass!

% make testacc TEST=./builtin/providers/dnsimple                                                                                            ✭
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/03/02 05:10:36 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/dnsimple -v  -timeout 120m
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccDNSimpleRecord_Basic
--- PASS: TestAccDNSimpleRecord_Basic (1.52s)
=== RUN   TestAccDNSimpleRecord_Updated
--- PASS: TestAccDNSimpleRecord_Updated (1.85s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/dnsimple	3.382s

Merged and will be part of 0.9 - it won't be back ported to 0.8.8 though if that's ok

Paul

weppos · 2017-03-02T09:14:44Z

Thanks @stack72. Yes, I'm absolutely fine with the changes NOT being back ported to 0.8.8.

joshuacronemeyer · 2017-03-10T20:00:56Z

I thought I'd give this great new code a try. Ran into a problem/inconsistency between the behavior you describe and what i'm actually seeing.

terraform --version                                                                         [ruby-2.2.1]
Terraform v0.9.0-dev (d24c761bbb96a9921575582a2055cccbfb0b302d+CHANGES)

provider "dnsimple" {
  token = "${var.dnsimple_token}"
  account = "${var.dnsimple_account}"
}

I added the token and got rid of the email settings as per the instructions in this PR, but when i run terraform plan i am immediately prompted for the dnsimple email

provider.dnsimple.email
  The DNSimple account email address.

  Enter a value:

My workaround is to configure email as empty string as follows

provider "dnsimple" {
  token = "${var.dnsimple_token}"
  account = "${var.dnsimple_account}"
  email = ""
}

If you actually set an email address then you get the following error

* module.events-staging.provider.dnsimple: DNSimple API v2 requires an account identifier and the new OAuth token. Please upgrade your configuration.

Maybe a problem with this PR. It wasn't immediately obvious to my what is causing it to prompt for the email. Hope this helps somebody.

stack72 · 2017-03-12T15:52:41Z

Hi @joshuacronemeyer

I am fixing this right now so that it will be part of the main release :) Thanks for testing this out for us

Paul

weppos · 2017-03-12T18:25:01Z

Thanks for reporting it @joshuacronemeyer

* Replace DNSimple API client with the official Go client * Upgrade DNSimple provider to use the new API v2 Acceptance tests pass: ``` === RUN TestProvider --- PASS: TestProvider (0.00s) === RUN TestProvider_impl --- PASS: TestProvider_impl (0.00s) === RUN TestAccDNSimpleRecord_Basic --- PASS: TestAccDNSimpleRecord_Basic (2.67s) === RUN TestAccDNSimpleRecord_Updated --- PASS: TestAccDNSimpleRecord_Updated (1.88s) PASS ok github.com/hashicorp/terraform/builtin/providers/dnsimple ``` Note that the code still has to be updated to pass the account ID dynamically in place of "TODO-ACCOUNT". * Refactor DNSimple provider to expose both client and config The config is required as the new API wants to know the identifier of the account you are operating to. The account is not stored in the client (as the client can talk with different accounts), hence I need to pass it as part of the config. * Identify Terraform requests to DNSimple via UserAgent * Upgrade to the latest dnsimple-go version * Update docs Provide upgrade instructions and update the docs for API v2. * Remove rendundant type declaration

joshuacronemeyer · 2017-03-13T17:47:10Z

Glad to hear it was useful. Thanks again.

ghost · 2020-04-15T02:31:01Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

stack72 self-assigned this Dec 15, 2016

stack72 added enhancement provider/dnsimple labels Dec 15, 2016

weppos added 4 commits March 1, 2017 17:18

Replace DNSimple API client with the official Go client

f14fee4

Identify Terraform requests to DNSimple via UserAgent

31d8688

weppos added 3 commits March 1, 2017 17:56

Upgrade to the latest dnsimple-go version

3069a5a

Update docs

727dfd9

Provide upgrade instructions and update the docs for API v2.

Remove rendundant type declaration

a8fa623

stack72 merged commit 8ae3174 into hashicorp:master Mar 2, 2017

weppos deleted the dnsimple-apiv2 branch March 2, 2017 09:14

yob mentioned this pull request Mar 18, 2017

Allow dnsimple_record.priority attribute to be set #9128

Closed

This was referenced Mar 18, 2017

Allow record priority to be specified when creating and updating pearkes/dnsimple#8

Closed

Allow dnsimple_record.priority attribute to be set #12843

Merged

Make dnsimple_records importable #9130

Merged

ghost locked and limited conversation to collaborators Apr 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade DNSimple provider to API v2 #10760

Upgrade DNSimple provider to API v2 #10760

weppos commented Dec 15, 2016 •

edited

Loading

stack72 commented Dec 15, 2016

weppos commented Dec 15, 2016

weppos commented Jan 28, 2017

weppos commented Feb 7, 2017

weppos commented Feb 27, 2017

stack72 commented Feb 27, 2017

weppos commented Feb 27, 2017

stack72 commented Feb 27, 2017

weppos commented Mar 1, 2017

weppos commented Mar 1, 2017

stack72 commented Mar 2, 2017

weppos commented Mar 2, 2017

joshuacronemeyer commented Mar 10, 2017

stack72 commented Mar 12, 2017

weppos commented Mar 12, 2017

joshuacronemeyer commented Mar 13, 2017

ghost commented Apr 15, 2020

Upgrade DNSimple provider to API v2 #10760

Upgrade DNSimple provider to API v2 #10760

Conversation

weppos commented Dec 15, 2016 • edited Loading

stack72 commented Dec 15, 2016

weppos commented Dec 15, 2016

weppos commented Jan 28, 2017

weppos commented Feb 7, 2017

weppos commented Feb 27, 2017

stack72 commented Feb 27, 2017

weppos commented Feb 27, 2017

stack72 commented Feb 27, 2017

weppos commented Mar 1, 2017

weppos commented Mar 1, 2017

stack72 commented Mar 2, 2017

weppos commented Mar 2, 2017

joshuacronemeyer commented Mar 10, 2017

stack72 commented Mar 12, 2017

weppos commented Mar 12, 2017

joshuacronemeyer commented Mar 13, 2017

ghost commented Apr 15, 2020

weppos commented Dec 15, 2016 •

edited

Loading