-
Notifications
You must be signed in to change notification settings - Fork 156
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
Fix screen widget FillMin and FillMax type matching #202
Conversation
@nyanshak / @ojongerius Mind reviewing this when you have a chance? |
@@ -96,10 +96,10 @@ type TileDefRequestStyle struct { | |||
} | |||
|
|||
type TileDefStyle struct { | |||
Palette *string `json:"palette,omitempty"` | |||
PaletteFlip *string `json:"paletteFlip,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should remain *string
because Datadog's API will accept strings or boolean values. If a user (through the json editor in the UI or via the API), sends "paletteFlip": "true"
, Datadog will accept and return that value in future calls as a string type, and the UI handles that correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nyanshak thanks for the prompt review. Updated the PR and squashed the commits too. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the integration tests to use the new types for FillMin
and FillMax
? Specifically, changing L315-316 in integration/screen_widgets_test.go
to use datadog.JsonNumber
rather than datadog.String
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks
Happy to accept this if we leave paletteFlip as a |
integration/screen_widgets_test.go
Outdated
@@ -4,7 +4,6 @@ import ( | |||
"testing" | |||
|
|||
"github.com/stretchr/testify/assert" | |||
"github.com/zorkian/go-datadog-api" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't remove this line, as it's needed to pull in types:
[go-datadog-api] make testacc 11:51:09 ☁ dabcoder-master ☂ ✭
go test integration/* -v -timeout 90m
# command-line-arguments [command-line-arguments.test]
integration/screen_widgets_test.go:10:15: undefined: datadog
FAIL command-line-arguments [build failed]
make: *** [Makefile:17: testacc] Error 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what happened there, didn't mean to remove it, but good catch, re-added it.
fillMax
andfillMix
can be decimals, so importing a screenboard that includes those withterraform import datadog_screenboard.<screenboard_name> <screenboard_id>
would throw an error like:This PR attempts to fix it, based on https://github.com/zorkian/go-datadog-api/blob/master/dashboards.go#L112-L113.
Attempts to fix the type forPaletteFlip
too.