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.

Sign up for GitHub

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

Jump to bottom

Add full support for timeseries definitions #282

Merged
merged 1 commit into from
Sep 4, 2019
Merged

Add full support for timeseries definitions #282

merged 1 commit into from
Sep 4, 2019

Conversation

zdylag
Copy link
Contributor

@zdylag zdylag commented Aug 9, 2019

  • Add support for Type
  • Add support for Events
  • Add support for YAxis

@ghost ghost added the size/M label Aug 9, 2019
@zdylag zdylag mentioned this pull request Aug 9, 2019
Closed
@bkabrda
Copy link
Contributor

bkabrda commented Aug 9, 2019

@zdylag thanks a lot for submitting this PR!

@enbashi since you're most familiar with dashboards resource, could you please review this?

enbashi
enbashi requested changes Aug 9, 2019
View reviewed changes
Copy link
Contributor

@enbashi enbashi left a comment

Choose a reason for hiding this comment

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

Thanks a lot @zdylag for this PR. I took a quick glance and added some comments. Could you also please add some tests for your changes? This will help a lot in reviewing.

datadog/resource_datadog_dashboard.go Outdated Show resolved Hide resolved
datadog/resource_datadog_dashboard.go Outdated Show resolved Hide resolved
datadog/resource_datadog_dashboard.go Outdated Show resolved Hide resolved
@zdylag
Copy link
Contributor Author

zdylag commented Aug 14, 2019

@enbashi I tried following the existing tests and adding some using that. Are there more tests I should add? I didn't see other tests that seemed similar?

jbenais
jbenais requested changes Aug 19, 2019
View reviewed changes
Copy link
Contributor

@jbenais jbenais left a comment

Choose a reason for hiding this comment

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

It looks like importing a dashboard with yaxis->include_zero set results to an error: widget.0.timeseries_definition.0.yaxis.include_zero: expected type 'string', got unconvertible type 'bool'.

datadog/resource_datadog_dashboard.go Outdated
},
},
"yaxis": {
Type: schema.TypeMap,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the type here. If you look at the HeatmapDefinitionSchema or ScatterplotDefinitionSchema that also have the yaxis field, the type used is TypeList.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jbenais I fixed up the yaxis to look more like the others, I think. Not sure how you are testing things. Does this work better now?

@nmuesch nmuesch mentioned this pull request Aug 20, 2019
Closed
jbenais
jbenais requested changes Aug 20, 2019
View reviewed changes
datadog/resource_datadog_dashboard.go Outdated Show resolved Hide resolved
datadog/resource_datadog_dashboard.go Outdated Show resolved Hide resolved
@zdylag zdylag force-pushed the master branch 2 times, most recently from 2a4bf98 to 2b65e05 Compare August 21, 2019 21:35
@zdylag
Copy link
Contributor Author

zdylag commented Aug 21, 2019

@jbenais thanks for the pointers! I copied the code in question and it maybe is happier now. Would you mind taking another look?

@stephen
Copy link

stephen commented Aug 26, 2019

hi @enbashi and @nmuesch, we're pretty excited to have these last pieces so that we can use the new dashboard api. could one of you take another look at this PR?

@enbashi
Copy link
Contributor

enbashi commented Aug 26, 2019

Thanks for the excitement, @stephen! We'll take a final pass at this during this week.

jbenais
jbenais approved these changes Aug 27, 2019
View reviewed changes
Copy link
Contributor

@jbenais jbenais left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for doing this! 💯💪🏻

datadog/resource_datadog_dashboard.go Outdated Show resolved Hide resolved
@zdylag
Copy link
Contributor Author

zdylag commented Aug 29, 2019

@jbenais @enbashi I addressed the nit. Could someone with write access merge this?

@stephen
Copy link

stephen commented Sep 3, 2019

hi @nmuesch sorry for pinging again - it looks like @jbenais's approval doesn't count now. could you review and merge this?

@nmuesch
Copy link
Contributor

nmuesch commented Sep 3, 2019

Hey, apologies I meant to tackle this today. Will take a look and hopefully merge tomorrow. Thanks again and sorry for the delay here.

@zdylag
Add full support for timeseries definitions
1d04cbe 
* Add support for Events
* Add support for YAxis
@zdylag
Copy link
Contributor Author

zdylag commented Sep 4, 2019

@nmuesch I removed the default line

@nmuesch
Copy link
Contributor

nmuesch commented Sep 4, 2019

Thanks a lot for the contribution and iterations! Going to merge this now! 🎉

@nmuesch nmuesch merged commit abf14d3 into DataDog:master Sep 4, 2019
@stephen
Copy link

stephen commented Sep 4, 2019

thank you! could you cut a new release with this change available?

@nmuesch
Copy link
Contributor

nmuesch commented Sep 4, 2019

There are a couple of other PRs open that I'll give a little time to go through. I think we can look at cutting a release early next week. 🙂

@nmuesch
Copy link
Contributor

nmuesch commented Sep 11, 2019

Hey, just wanted to close the loop here and let you know that 2.4.0 of the provider was released today that included this change. - https://github.com/terraform-providers/terraform-provider-datadog/blob/master/CHANGELOG.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enbashi enbashi enbashi requested changes

@jbenais jbenais jbenais approved these changes

@nmuesch nmuesch nmuesch approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@zdylag @bkabrda @stephen @enbashi @nmuesch @jbenais