[dashboards] Add new Dashboard resource #249

enbashi · 2019-06-26T19:43:52Z

Adds a new datadog_dashboard resource that uses the new Dashboard API https://docs.datadoghq.com/api/#dashboards

Merge upstream repo

Merge upstream

bkabrda

Overall this is awesome work, thanks a lot for doing this. Couple comments:

I've left several notes inline the code that should be addressed.
We'll need documentation for this feature under the website directory; while writing it, you can use make website to spin up a local Docker container to see if everything is formatted/linked right.

Also thanks for writing the integration tests - they pass for me and I've also been able to play around with this without any issues at all, so great job 👍

datadog/resource_datadog_dashboard.go

datadog/resource_datadog_dashboard_test.go

datadog/resource_datadog_dashboard.go

enbashi · 2019-06-27T17:11:58Z

Overall this is awesome work, thanks a lot for doing this. Couple comments:

I've left several notes inline the code that should be addressed.

We'll need documentation for this feature under the website directory; while writing it, you can use make website to spin up a local Docker container to see if everything is formatted/linked right.

Also thanks for writing the integration tests - they pass for me and I've also been able to play around with this without any issues at all, so great job 👍

Thanks @bkabrda for the review and comments. I've addressed them and would love to hear your thoughts about ConflictsWith limitations.
I am also working on a separate PR to update the docs. Would you like me to include the docs in this same PR instead?

bkabrda · 2019-06-27T17:13:56Z

@enbashi as for the ConflictsWith, I've actually already marked that as resolved because you correctly pointed out that my proposal wasn't viable.

As for docs, I would like to have them in the same PR.

Sync with upstream

enbashi · 2019-06-28T21:03:00Z

@bkabrda I added the docs.

bkabrda

Nice work on the docs; I left couple comments inline that should be addressed before we can merge this.

bkabrda · 2019-07-02T07:54:16Z

website/datadog.erb

@@ -34,15 +37,9 @@
            <li<%= sidebar_current("docs-datadog-resource-monitor") %>>
              <a href="/docs/providers/datadog/r/monitor.html">datadog_monitor</a>
            </li>
-            <li<%= sidebar_current("docs-datadog-resource-screenboard") %>>


Let's not remove the links to screenboard and timeboard resources just yet. Marking them outdated is sufficient for now IMO.

We wanted to follow the same approach we did for https://docs.datadoghq.com/api/#dashboards when we rolled out the new API to achieve two goals:

Push users to use the new resource while still having access to the old resources

Minimize confusion that might be caused by having three related resources accessible from the main page

That said, I don't have a strong opinion about that so if you still think we should keep the links I am happy add them back. let me know.

I think it would be really confusing if the documentation just disappeared one day without the users understanding why. There's no good way in TF provider docs to go back to docs of older versions, so keeping the links there and putting the disclaimers there for screenboard and timeboard resources seems like the best solution to me.

Since these won't be deprecated in the next immediate provider version, I also think we should keep the links. Once we start deprecation on the resources we should remove the links and log a notice. For now I think we should just leave a note at the top of each screenboard/timeboard page.

You can even use this to make it a big note !> at the top of the timeboard/screenboard pages. https://github.com/hashicorp/terraform-website#callouts (Just tested locally and confirmed it works. There is a make website command that will spin up a local copy of the site in docker)

Sure. I added the links back. Thank you both.

bkabrda · 2019-07-02T08:05:58Z

website/docs/r/dashboard.html.markdown

+    }
+  }
+
+  template_variable {


The indentation seems to be a bit off from this line on - let's continue using two spaces to indent as you did above.

Thanks for catching this. Fixed.

bkabrda · 2019-07-02T08:06:47Z

website/docs/r/dashboard.html.markdown

+}
+
+# Create a new Datadog dashboard - Free layout
+resource "datadog_dashboard" "free_dashboard" {


Indentation in this example is a little weird as well, let's keep using two spaces for each level of indentation.

bkabrda · 2019-07-02T08:24:37Z

website/docs/r/dashboard.html.markdown

+  - `limit` - (Required)
+
+
+### Nested `widget``marker` blocks


Missing a space here between widget and marker, would result in bad formatting.

bkabrda · 2019-07-02T08:25:09Z

website/docs/r/dashboard.html.markdown

+
+Only for widgets of type "timeseries".
+
+Nested `widget``marker` blocks have the following structure:


Again missing space between widget and marker.

bkabrda

LGTM now, great work 👍 . Since this is a big one, I'd also like one other approval. @nmuesch could you add your approval as well if you're ok with everything?

stephen · 2019-07-10T17:19:48Z

hi, just wanted to say that this has been my #1 datadog feature request and i'm excited to see it happen.

@nmuesch can i provide any motivation to get this reviewed and shipped?

nmuesch · 2019-07-17T16:22:03Z

Hey. Glad to see this and that its being looked forward to! Apologies for my delay here. I'll be taking a look at this shortly and leaving a review!

nmuesch

Thanks again for this! 💜 Tests/Docs especially. I left a few small comments, but the primary ones I think should get addressed before we merge are:

The clarification about two build functions per nested schema
Can you try to reproduce the issue with the non empty plan when updating the example provided in the docs?

nmuesch · 2019-07-17T17:00:59Z

website/docs/r/dashboard.html.markdown

+  }
+}
+
+# Create a new Datadog dashboard - Free layout


I think this would look cleaner as a separate code block with this comment maybe being an anchor title?

Agreed. This looks much better. Thanks for the suggestion. Updated.

nmuesch · 2019-07-19T18:35:19Z

datadog/resource_datadog_dashboard.go

+	"strings"
+
+	"github.com/hashicorp/terraform/helper/schema"
+	"github.com/zorkian/go-datadog-api"


Should this be aliased to datadog ?

nmuesch · 2019-07-19T18:37:51Z

datadog/resource_datadog_dashboard.go

+			"layout_type": {
+				Type:        schema.TypeString,
+				Required:    true,
+				Description: "The layout type of the dashboard, either 'free' or 'ordered'.",


What do you think about having a ValidateFunc here to ensure a user provided one of the two expected values - https://www.terraform.io/docs/extend/schemas/schema-behaviors.html#validatefunc

I don't anticipate these options changing soon :)

I was relying on the upstream API to handle validation in some cases to simplify the resource code, but this could be a nice candidate for ValidateFunc. Updated.

nmuesch · 2019-07-19T18:44:29Z

datadog/resource_datadog_dashboard.go

+			terraformRequest["apm_query"] = []map[string]interface{}{terraformQuery}
+		} else if datadogRequest.LogQuery != nil {
+			terraformQuery := buildTerraformApmOrLogQuery(*datadogRequest.LogQuery)
+


It looks like something is up here. Running terraform apply on the documented dashboard example multiple times gives me a non empty diff.

For example:

widget.10.scatterplot_definition.0.xaxis.include_zero: "true" => "1"

That one was super tricky. It seems that all params in TypeMap are stored in the local state as strings regardless of the provided param type. I had to change the type of the surrounding blocks xaxis and yaxis to TypeList to workaround around that. The docs and tests were updated accordingly. Thanks for catching this.

datadog/resource_datadog_dashboard.go

enbashi · 2019-07-23T19:21:47Z

datadog/resource_datadog_dashboard.go

+}
+func validateGroupWidgetLayoutType(val interface{}, key string) (warns []string, errs []error) {
+	value := val.(string)
+	switch value {


Adding a switch despite this being one value only because we plan to support more layout types in the future.

nmuesch

Overall this looks good to me. Thanks for tackling this and the reviews 🙇

One final thing I notice but don't think should block a merge. There is a (likely expected) behavior that when a group of widgets is moved via the UI, the next terraform apply will show a very large diff. Though I don't see a way around this.

bkabrda · 2019-07-24T09:55:57Z

@nmuesch yeah, I don't think we can't help that, but nice catch :)

All right, everything seems to be in line here. I'm merging!

enbashi and others added 3 commits May 21, 2019 13:49

Merge pull request #2 from terraform-providers/master

b56b7ca

Merge upstream repo

Merge pull request #3 from terraform-providers/master

7692051

Merge upstream

Add new Dashboard resource

cb96bc6

ghost added the size/XXL label Jun 26, 2019

bkabrda suggested changes Jun 27, 2019

View reviewed changes

datadog/resource_datadog_dashboard.go Outdated Show resolved Hide resolved

datadog/resource_datadog_dashboard_test.go Outdated Show resolved Hide resolved

datadog/resource_datadog_dashboard.go Show resolved Hide resolved

Fix tests + review feedback

6538824

Add datadog_dashboard docs

eba726a

ghost added the documentation label Jun 28, 2019

enbashi and others added 2 commits June 28, 2019 16:57

Merge pull request #4 from terraform-providers/master

9a9b481

Sync with upstream

Resolve conflicts

5503a93

bkabrda suggested changes Jul 2, 2019

View reviewed changes

Update docs with review comments

4926cad

bkabrda approved these changes Jul 3, 2019

View reviewed changes

Update inner Note widget example in tests and docs

b73f8bc

nmuesch requested changes Jul 19, 2019

View reviewed changes

enbashi added 4 commits July 23, 2019 14:03

Use TypeList for axis to correctly parse bool values

1088f7b

Add ValidateFunc for layout_type

76457b1

Add aliased import

62b8a3e

Split examples in docs into two blocks

75465a5

enbashi commented Jul 23, 2019

View reviewed changes

Update examples to not use optional params not used in the UI

0d483ad

nmuesch approved these changes Jul 23, 2019

View reviewed changes

bkabrda merged commit 236dded into DataDog:master Jul 24, 2019

bkabrda mentioned this pull request Jul 24, 2019

Support new dashboard API #211

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dashboards] Add new Dashboard resource #249

[dashboards] Add new Dashboard resource #249

enbashi commented Jun 26, 2019

bkabrda left a comment

enbashi commented Jun 27, 2019

bkabrda commented Jun 27, 2019

enbashi commented Jun 28, 2019

bkabrda left a comment

bkabrda Jul 2, 2019

enbashi Jul 2, 2019

bkabrda Jul 2, 2019

nmuesch Jul 2, 2019

enbashi Jul 2, 2019

bkabrda Jul 2, 2019

enbashi Jul 2, 2019

bkabrda Jul 2, 2019

bkabrda Jul 2, 2019

bkabrda Jul 2, 2019

bkabrda left a comment

stephen commented Jul 10, 2019

nmuesch commented Jul 17, 2019

nmuesch left a comment

nmuesch Jul 17, 2019

enbashi Jul 23, 2019

nmuesch Jul 19, 2019

enbashi Jul 23, 2019

nmuesch Jul 19, 2019

enbashi Jul 23, 2019

nmuesch Jul 19, 2019

enbashi Jul 23, 2019

enbashi Jul 23, 2019

nmuesch left a comment

bkabrda commented Jul 24, 2019


		Only for widgets of type "timeseries".

		Nested `widget``marker` blocks have the following structure:

[dashboards] Add new Dashboard resource #249

[dashboards] Add new Dashboard resource #249

Conversation

enbashi commented Jun 26, 2019

bkabrda left a comment

Choose a reason for hiding this comment

enbashi commented Jun 27, 2019

bkabrda commented Jun 27, 2019

enbashi commented Jun 28, 2019

bkabrda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkabrda left a comment

Choose a reason for hiding this comment

stephen commented Jul 10, 2019

nmuesch commented Jul 17, 2019

nmuesch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmuesch left a comment

Choose a reason for hiding this comment

bkabrda commented Jul 24, 2019