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

Allow both int and string in more places #219

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Allow both int and string in more places #219

wants to merge 11 commits into from

Conversation

mrwonko
Copy link

@mrwonko mrwonko commented Mar 1, 2019 •
edited
Loading

This is basically a combination of the apparently abandoned #198 and #195 with some additional cleanup and added tests. The goal is fixing #192.

With the fixes in place I noticed additional type mismatches for Params.Count, Params.Start and ConditionalFormat.Value, which I fixed in the same way.

@mrwonko mrwonko changed the title Allow both int and string for Screenboard size Allow both int and string in more places Mar 1, 2019
@mrwonko
Copy link
Author

mrwonko commented Mar 1, 2019

Still need to fix

json: cannot unmarshal bool into Go struct field TileDefStyle.paletteFlip of type string

json: cannot unmarshal string into Go struct field Widget.alert_id of type int

and maybe #220:

json: cannot unmarshal bool into Go struct field TileDefMarker.label of type string

(althought that one doesn't block me quite as much)

ojongerius
ojongerius requested changes Mar 3, 2019
View reviewed changes
Copy link
Collaborator

@ojongerius ojongerius left a comment

Choose a reason for hiding this comment

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

Nice work!

common.go Outdated Show resolved Hide resolved
datadog-accessors.go Show resolved Hide resolved
helpers.go Outdated Show resolved Hide resolved
helpers.go Outdated Show resolved Hide resolved
screenboards_test.go Outdated Show resolved Hide resolved
@ojongerius
Copy link
Collaborator

Still need to fix

json: cannot unmarshal bool into Go struct field TileDefStyle.paletteFlip of type string

json: cannot unmarshal string into Go struct field Widget.alert_id of type int

and maybe #220:

json: cannot unmarshal bool into Go struct field TileDefMarker.label of type string

(althought that one doesn't block me quite as much)

@mrwonko thanks so much for all this work! If you are happy to address those issues too, that would be fantastic.

@mrwonko
Copy link
Author

mrwonko commented Mar 4, 2019

Still need to fix

json: cannot unmarshal bool into Go struct field TileDefStyle.paletteFlip of type string

json: cannot unmarshal string into Go struct field Widget.alert_id of type int

and maybe #220:

json: cannot unmarshal bool into Go struct field TileDefMarker.label of type string

(althought that one doesn't block me quite as much)

@mrwonko thanks so much for all this work! If you are happy to address those issues too, that would be fantastic.

Will do, I just ran out of time on Friday.

mrwonko and others added 10 commits March 4, 2019 09:25
@mrwonko
Improve Screen- and Dashboard tests
02a01a8 
- Use Client of httptest Server
- Set RetryTimeout in datadogClient so it doesn't retry indefinitely on error
- Move testdata into the well-known testdata/ folder so test caching etc. works
  properly
- Return error from mock Datadog server instead of calling t.Fatal, which is
  only allowed in the test's main Goroutine
- Use reflect.DeepEqual to shorten Screenboard test
@teraken0509 @mrwonko
Rename PrecisionT to StrIntD and add tests
3d26c69 
We'll use that type for other things besides precision.
@mrwonko
Accepts ints and strings for Screenboard size.
61e10ed 
Besides integer sizes in pixel (e.g. 768) the API can also return percentages
(e.g. "100%").

Fixes zorkian#192
@mrwonko
Accept integers for Widget.Params.Count and .Start
1a4d4a2
@mrwonko
Accept integers for ConditionalFormat.Value
32436b2
@mrwonko
Allow floating point Widget positions
9227dda 
They happen.
@mrwonko
Accepts ints and strings for manage_status titleSize
4c4edea
@mrwonko
Accept bools and strings as Marker Labels
25df54e 
This introduces a new type StrBoolD (similar to StrIntD) for types that can be
either strings or bools in JSON.

Fixes zorkian#220
@mrwonko
Accept bools and strings as Style.PaletteFlip
ff651de
@mrwonko
Accept strings and integers as AlertID
b04c74e
@mrwonko
Copy link
Author

mrwonko commented Mar 5, 2019

As far as I'm concerned this is now done, all the screenboards from my project work now.

@ojongerius
Copy link
Collaborator

Thanks @mrwonko! I'll have some time to review and hopefully merge tomorrow.

@ojongerius
Copy link
Collaborator

@masci as a Datadog employee with most likely more insight in your API, what is your opinion on this?

While I very much appreciate the library working around quirks of the DD API, it is becoming so inconsistent. I'd hate to continue to introduce breaking changes each time people find them.

Any ideas on a structural approach to this problem, @mrwonko, @masci. @nyanshak, @yfronto or @zorkian, care to think along?

@codezninja codezninja mentioned this pull request Mar 12, 2019
Closed
@facundovictor
Copy link

facundovictor commented Jun 5, 2019 •
edited
Loading

@ojongerius, @mrwonko What is the status update of this? would this fix https://github.com/terraform-providers/terraform-provider-datadog/issues/105?

@nmuesch
Copy link
Collaborator

nmuesch commented Jun 6, 2019

Hey @ojongerius From Datadog here. I've communicated internally to put these issues on the radar but don't have any timeframes for when a longer term fix would be possible unfortunately.

I'm in favor of pushing this forward if you are, to resolve the existing issues people are facing.

@ryanschneider
Copy link

Is this PR still likely to get merged? Just ran into the json: cannot unmarshal number into Go struct field Params.widgets.params.count of type string error today when trying to update a screen board widget programmatically.

@onematchfox
Copy link

onematchfox commented Dec 11, 2019 •
edited
Loading

@ojongerius Also wondering what the status on this one is as it seems to have been dormant for >9 months now? Ran into the problem today as I'm no longer able to update a screenboard with container tiles using the terraform provider due to the error err: json: cannot unmarshal bool into Go struct field TileDefStyle.paletteFlip of type string. Any chance this will be merged at some point?

@matt-canty-dragon matt-canty-dragon mentioned this pull request Mar 17, 2020
Closed
@afarbos
Copy link

afarbos commented Jun 25, 2020 •
edited
Loading

It seems everybody is in favor, I am not sure to understand why this is not merged @ojongerius
Note: It also seems this relates with my current opened PR: #325.

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

@ojongerius ojongerius ojongerius requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mrwonko @ojongerius @facundovictor @nmuesch @ryanschneider @onematchfox @afarbos @teraken0509