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 custum_bg_color to screenboard #189

Merged
merged 2 commits into from
May 21, 2019

Conversation

milanvdm
Copy link
Contributor

@milanvdm milanvdm commented May 1, 2019

In the documentation it is mentioned that custom_bg is supported for a screenboard (https://www.terraform.io/docs/providers/datadog/r/screenboard.html#palette-2).

But no custom_bg_color field is available such as https://www.terraform.io/docs/providers/datadog/r/timeboard.html#custom_bg_color.

I added the field to the screenboard code.

@ghost ghost added the size/XS label May 1, 2019
@bkabrda
Copy link
Contributor

bkabrda commented May 14, 2019

Thanks for the PR!
Unfortunately this doesn't seem to work for me. AFAICS you would need to add custom_bg_color to buildTFTileDefRequestConditionalFormats and in order to do that you'd need to add that property to the go-datadog-api library [1] (to ConditionalFormat in screen_widgets.go). I think that should be enough to make it work. If you do want to do that, I can also review your PR for [1] as I have commit rights there - and then we could get back to this PR to get it fixed.

[1] https://github.com/zorkian/go-datadog-api

@milanvdm
Copy link
Contributor Author

@bkabrda Thanks for the information, will have a look at it and update my PR accordingly :)

@milanvdm
Copy link
Contributor Author

@bkabrda There you go zorkian/go-datadog-api#242
Keep me posted on how the release process goes as I assume Ill need to update the api-version here to be able to use those changes.

@bkabrda
Copy link
Contributor

bkabrda commented May 21, 2019

@milanvdm feel free to just run:

go get -u github.com/zorkian/go-datadog-api@master
go mod vendor

And add another commit with the resulting diff to update the dep - that's what we do most of the time and it works fine. Alternatively, you could open a standalone PR to update the deps, but I think adding it in a different commit of this PR is ok.

@milanvdm
Copy link
Contributor Author

@bkabrda Thanks for giving me the go commands ;)

@bkabrda
Copy link
Contributor

bkabrda commented May 21, 2019

@milanvdm no problem! Now as I suggested in one of my previous comments, you'll need to adjust buildTFTileDefRequestConditionalFormats and also buildTileDefRequestsConditionalFormats to set these properly. Let me know if you need any help with that.

@bkabrda
Copy link
Contributor

bkabrda commented May 21, 2019

Looks real good now, but you'll need to apply this diff to make the tests pass:

diff --git a/datadog/resource_datadog_screenboard_test.go b/datadog/resource_datadog_screenboard_test.go
index 2bf93e3..8a9ad86 100644
--- a/datadog/resource_datadog_screenboard_test.go
+++ b/datadog/resource_datadog_screenboard_test.go
@@ -1189,7 +1189,7 @@ func TestAccDatadogScreenboard_update(t *testing.T) {
                        resource.TestCheckResourceAttr("datadog_screenboard.acceptance_test", "widget.2.tile_def.0.request.0.aggregator", "max"),
                        resource.TestCheckResourceAttr("datadog_screenboard.acceptance_test", "widget.2.tile_def.0.request.0.change_type", ""),
                        resource.TestCheckResourceAttr("datadog_screenboard.acceptance_test", "widget.2.tile_def.0.request.0.compare_to", ""),
-                       resource.TestCheckResourceAttr("datadog_screenboard.acceptance_test", "widget.2.tile_def.0.request.0.conditional_format.#", "2"),
+                       resource.TestCheckResourceAttr("datadog_screenboard.acceptance_test", "widget.2.tile_def.0.request.0.conditional_format.#", "3"),
                        resource.TestCheckResourceAttr("datadog_screenboard.acceptance_test", "widget.2.tile_def.0.request.0.conditional_format.0.color", ""),
                        resource.TestCheckResourceAttr("datadog_screenboard.acceptance_test", "widget.2.tile_def.0.request.0.conditional_format.0.comparator", ">"),
                        resource.TestCheckResourceAttr("datadog_screenboard.acceptance_test", "widget.2.tile_def.0.request.0.conditional_format.0.invert", "false"),

Once you do that, I'll merge this PR.

@bkabrda
Copy link
Contributor

bkabrda commented May 21, 2019

Also, please consider squashing all the commits except the one that updates the deps, it would help improve the commit history of the repo when I merge.

@milanvdm
Copy link
Contributor Author

@bkabrda I'm afraid you lost me with applying this diff. This does not make sense to me in my Scala world.

@bkabrda
Copy link
Contributor

bkabrda commented May 21, 2019

@milanvdm sorry for the confusion. Basically there's one more line left over to fix in the datadog/resource_datadog_screenboard_test.go for the acceptance tests to pass now - that's what the diff shows. You modified the conditional format of that widget by adding one more element, but forgot to update the test that expects a certain number of elements to be ignored (it expects 2 but you need to change it to 3 to accomodate for the element you added).

@milanvdm milanvdm force-pushed the add-custom-bg-color-screenboard branch from 5bb3416 to 832056a Compare May 21, 2019 13:22
@ghost ghost added size/XXL and removed size/L labels May 21, 2019
@milanvdm milanvdm closed this May 21, 2019
@milanvdm milanvdm force-pushed the add-custom-bg-color-screenboard branch from 832056a to 8f03724 Compare May 21, 2019 13:32
@ghost ghost added size/XS and removed size/XXL labels May 21, 2019
@milanvdm milanvdm reopened this May 21, 2019
@milanvdm
Copy link
Contributor Author

@bkabrda Fixed the tests and squashed my commits.

@bkabrda
Copy link
Contributor

bkabrda commented May 21, 2019

Perfect work, I'm merging this PR now. Thank you so much for all your work on this!

@bkabrda bkabrda merged commit 098745d into DataDog:master May 21, 2019
@milanvdm milanvdm deleted the add-custom-bg-color-screenboard branch May 21, 2019 14:22
@milanvdm
Copy link
Contributor Author

@bkabrda Thanks a lot for helping me out with Go!
How does the release process of this provider work?

@bkabrda
Copy link
Contributor

bkabrda commented May 21, 2019

The release process (in terms of when a new release happens) doesn't have any strict rules. Before version 1.8.0, the releases weren't too often but now we're managing to do more changes than we used to, so it's likely that it will happen in a week or two (but I can't guarantee that 100 %).

@milanvdm
Copy link
Contributor Author

@bkabrda Is there any way so I can use a certain commit of a provider or an RC release?

@bkabrda
Copy link
Contributor

bkabrda commented May 22, 2019

@milanvdm personally, I use a very specific way that I would never recommend for actual production use - I compile the provider myself (go install) and then run terraform init --plugin-dir=<dirWithCompiledProvider. The disadvantage here is that now terraform will expect all plugins to reside in that directory - for a setup where you're using more providers, this wouldn't be too practical.

I guess you could follow https://www.terraform.io/docs/commands/init.html#plugin-installation - add the compiled provider to ~/.terraform.d/plugins and rerun terraform init - IIUC this should work, but I've actually never tried.

A word of warning - while we do try as much as we can to make sure every merged PR doesn't break anything, we sometimes hit minor bugs when we do pre-release testing. So please make sure you double check everything that's happening with your TF plan.

Hope this helps!

jbenais pushed a commit to jbenais/terraform-provider-datadog that referenced this pull request Aug 20, 2019
…enboard

Add custum_bg_color to screenboard
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants