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

provider/datadog: Upgrade to Datadog API v2 #12098

Merged
merged 4 commits into from
Feb 20, 2017

Conversation

ojongerius
Copy link
Contributor

This pulls in v2 of https://github.com/zorkian/go-datadog-api. I suspect this will allow us to fix some bugs in the provider as we can now distinguish between set and unset options. See this zorkian/go-datadog-api#56 for discussion of why and how we did this.

This PR covers switching to v2, and updating the provider to use it, as it contained breaking API changes. I've done the minimal amount of work possible, the core benefits of this work are:

  • Considerable speed improvement; v2 includes a bug fix that runs the sum of all available integration tests less time that one test used to take.
  • The grunt work needed to start fixing bugs related to default types.

Before changes, TestAccDatadogMonitor_BasicNoTreshold took 92.38s. Afterwards the sum of all tests took 65.189s.

Before:

▶ make testacc TEST=./builtin/providers/datadog TESTARGS='-run=TestAccDatadogMonitor_Basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/02/20 11:23:51 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/datadog -v -run=TestAccDatadogMonitor_Basic -timeout 120m
=== RUN   TestAccDatadogMonitor_Basic

--- PASS: TestAccDatadogMonitor_Basic (92.52s)
=== RUN   TestAccDatadogMonitor_BasicNoTreshold
--- PASS: TestAccDatadogMonitor_BasicNoTreshold (92.38s)
<SNIP>

After:

▶ make testacc TEST=./builtin/providers/datadog
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/02/20 15:51:27 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/datadog -v  -timeout 120m
=== RUN   TestDatadogMonitor_import
--- PASS: TestDatadogMonitor_import (7.95s)
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccDatadogMonitor_Basic
--- PASS: TestAccDatadogMonitor_Basic (5.79s)
=== RUN   TestAccDatadogMonitor_BasicNoTreshold
--- PASS: TestAccDatadogMonitor_BasicNoTreshold (6.17s)
=== RUN   TestAccDatadogMonitor_Updated
--- PASS: TestAccDatadogMonitor_Updated (12.02s)
=== RUN   TestAccDatadogMonitor_TrimWhitespace
--- PASS: TestAccDatadogMonitor_TrimWhitespace (6.73s)
=== RUN   TestAccDatadogMonitor_Basic_float_int
--- PASS: TestAccDatadogMonitor_Basic_float_int (11.25s)
=== RUN   TestAccDatadogTimeboard_update
--- PASS: TestAccDatadogTimeboard_update (15.27s)
=== RUN   TestValidateAggregatorMethod
--- PASS: TestValidateAggregatorMethod (0.00s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/datadog        65.189s

See the README of v2 for usage of the API.

@bedwards and @nyanshak you've done the work on resourceDatadogTimeboard. Are you able to test this? I just ran the acceptance tests and things look ok.

I do plan on refactoring the Graph struct in the library by breaking it apart. We now generate the accessors, but only for top level structs. This is a deliberate choice and I think the library would benefit from that work. If that is done I, or someone else, could simplify the dashboard resource a little.

    See zorkian/go-datadog-api#56 for context.

    * Fixes bug in backoff implementation that decreased performance significantly.
    * Uses pointers for field types, providing support of distinguishing
        between if a value is set, or the default value for that type is
        effective.
@@ -373,9 +371,9 @@ func appendRequests(datadogGraph *datadog.Graph, terraformRequests *[]interface{
func appendEvents(datadogGraph *datadog.Graph, terraformEvents *[]interface{}) {
for _, t_ := range *terraformEvents {
Copy link
Contributor Author

@ojongerius ojongerius Feb 20, 2017

Choose a reason for hiding this comment

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

One of the Graph nested structs that would be nicer to use if it would be broken apart. Have raised zorkian/go-datadog-api#98 to do this.

if autoscale, ok := t["autoscale"]; ok {
d.Definition.Autoscale = autoscale.(bool)
if v, ok := t["autoscale"]; ok {
d.Definition.Autoscale = datadog.Bool(v.(bool))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think variables that live no longer than 2 lines are worth it. Apart from that I find this easier to parse.

if palette_, ok := s["palette"]; ok {
palette := palette_.(string)
if v, ok := s["palette"]; ok {
palette := v.(string)
style.Palette = &palette
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the Graph struct has been broken up in zorkian/go-datadog-api#98 we'll have generated Setters that do this tedious and error prone work for us.

@stack72
Copy link
Contributor

stack72 commented Feb 20, 2017

Nice one @ojongerius :) Does this get rid of the need for #12086?

@stack72
Copy link
Contributor

stack72 commented Feb 20, 2017

This LGTM :)

% make testacc TEST=./builtin/providers/datadog
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/02/20 14:46:54 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/datadog -v  -timeout 120m
=== RUN   TestDatadogMonitor_import
--- PASS: TestDatadogMonitor_import (4.84s)
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccDatadogMonitor_Basic
--- PASS: TestAccDatadogMonitor_Basic (3.70s)
=== RUN   TestAccDatadogMonitor_BasicNoTreshold
--- PASS: TestAccDatadogMonitor_BasicNoTreshold (3.60s)
=== RUN   TestAccDatadogMonitor_Updated
--- PASS: TestAccDatadogMonitor_Updated (6.39s)
=== RUN   TestAccDatadogMonitor_TrimWhitespace
--- PASS: TestAccDatadogMonitor_TrimWhitespace (3.38s)
=== RUN   TestAccDatadogMonitor_Basic_float_int
--- PASS: TestAccDatadogMonitor_Basic_float_int (7.35s)
=== RUN   TestAccDatadogTimeboard_update
--- PASS: TestAccDatadogTimeboard_update (9.45s)
=== RUN   TestValidateAggregatorMethod
--- PASS: TestValidateAggregatorMethod (0.00s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/datadog	38.729s

@stack72 stack72 merged commit 2310316 into hashicorp:master Feb 20, 2017
stack72 pushed a commit that referenced this pull request Feb 20, 2017
* provider/datadog: Pulls v2 and removes v1 of library go-datadog-api.

    See zorkian/go-datadog-api#56 for context.

    * Fixes bug in backoff implementation that decreased performance significantly.
    * Uses pointers for field types, providing support of distinguishing
        between if a value is set, or the default value for that type is
        effective.

* provider/datadog: Convert provider to use v2 of go-datadog-api.

* provider/datadog: Update vendored library.

* provider/datadog: Update dashboard resource to reflect API updates.
@ojongerius ojongerius deleted the ojongerius/go-datadog-api-v2 branch February 20, 2017 22:55
@ojongerius
Copy link
Contributor Author

ojongerius commented Feb 20, 2017

Cheers for the fast turnaround @stack72! Replied to your question in #12086

@ghost
Copy link

ghost commented Apr 16, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants