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

Fix Screenboard support #168

Merged
merged 5 commits into from
Sep 4, 2018
Merged

Fix Screenboard support #168

merged 5 commits into from
Sep 4, 2018

Conversation

anatolebeuzon
Copy link
Contributor

@anatolebeuzon anatolebeuzon commented Sep 4, 2018 •
edited
Loading

Datadog engineer here 👋

As explained in #32, the support for Datadog screenboards was broken. This PR adresses this issue.

Things changed

  • Some struct fields have been removed to match removed attributes in the API, and some struct fields have been added to add support for new widgets. All 19 screenboard widgets have been tested sucessfully.
  • The multiple screenboard widgets have been merged into a single one, to avoid redundancy and to match more closely the actual API (which accepts pretty much any JSON)
  • Integration tests for screenboard widgets have been removed. Indeed, they would not really test anything since the API accepts arbitrary JSON.

@anatolebeuzon anatolebeuzon mentioned this pull request Sep 4, 2018
Closed
nyanshak
nyanshak requested changes Sep 4, 2018
View reviewed changes
Copy link
Collaborator

@nyanshak nyanshak left a comment

Choose a reason for hiding this comment

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

Please add integration tests, longer comment left on the issue itself.

nyanshak
nyanshak reviewed Sep 4, 2018
View reviewed changes
screen_widgets.go Outdated
Group *string `json:"group,omitempty"`
Grouping *string `json:"grouping,omitempty"`
type Time struct {
LiveSpan *string `mapstructure:"live_span" json:"live_span,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no reference to the mapstructure package in this repo. What's the intent of this tag? Will it be consumed downstream?

cc @yfronto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, didn't add this on purpose. Leftover from #158. Nice catch!

@ghost
Copy link

ghost commented Sep 4, 2018

Thanks @oxlay! lgtm

@nyanshak nyanshak merged commit 05fc4dd into zorkian:master Sep 4, 2018
@bkabrda bkabrda mentioned this pull request Feb 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyanshak nyanshak nyanshak approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@anatolebeuzon @nyanshak