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

[dashboard list] Add new endpoints + dog cmd for dashboard lists #252

Merged
merged 5 commits into from
Mar 16, 2018

Conversation

MLaureB
Copy link
Contributor

@MLaureB MLaureB commented Feb 7, 2018

This PR adds the new API endpoints for managing dashboard lists:

  • Get all dashboard lists: GET dashboard/lists/manual

  • Get dashboard list: GET dashboard/lists/manual/{dashboard_list_id}

  • Create dashboard list: POST dashboard/lists/manual

  • Update dashboard list: PUT dashboard/lists/manual/{dashboard_list_id}

  • Delete dashboard list: DELETE dashboard/lists/manual/{dashboard_list_id}

  • Get dashboards for dashboard list: GET dashboard/lists/manual/{dashboard_list_id}/dashboards

  • Add dashboards to dashboard list: POST dashboard/lists/manual/{dashboard_list_id}/dashboards

  • Update dashboards of dashboard list: PUT dashboard/lists/manual/{dashboard_list_id}/dashboards

  • Delete dashboards from dashboard list: DELETE dashboard/lists/manual/{dashboard_list_id}/dashboards

:returns: Dictionary representing the API's JSON response
"""
return super(DashboardList, cls)._trigger_class_action('GET', 'dashboards', id, params)

Copy link
Member

Choose a reason for hiding this comment

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

I think we should add a PUTmethod (.. PATCH ideally but it's better to be consistent with other api).
This would allow people to modify dashboards in a list in an atomic way, without callling delete first. It comes in handy when people have lists stored somewhere in source control and want to make sure Datadog resources are in sync with what they have.

@@ -8,7 +8,7 @@ class Host(ActionAPIResource):
_class_url = '/host'

@classmethod
def mute(cls, host_name, **params):
def mute(cls, host_name, **body):
Copy link
Member

Choose a reason for hiding this comment

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

👍

report_warnings(res)
report_errors(res)

if args.string_ids:
Copy link
Member

@iddl iddl Feb 8, 2018

Choose a reason for hiding this comment

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

I've chatted with @yannmh about this logic, it's definitely a legacy behavior, I think there is little benefit in adding more instances of it. Let's not include it.
I see it's used in timeboard screenboard monitor downtime, we can handle those separately.

Copy link
Member

@yannmh yannmh left a comment

Choose a reason for hiding this comment

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

When moving from the dogapi to the datadogpy Python library, we forced all the resources to share the same interface, defined by the (Creatable|Updatable|..)APIResources classes for CRUD operations.
The result is a more homegeneous interface, i.e.

api.Dashboard.create(...)
api.Tag.create(...)
api.Graph.create(...)

instead of something like (in dogapi)

api.create_dashboard(...)
api.add_tag_to_host(...)
api.make_snapshot(...)

I feel like the DashboardList resource interface "breaks" with the current design by adding more CRUD methods on top of the standard (create|update|get|delete) ones. The result is a bit confusing:

api.DashboardList.get(...)
api.DashboardList.get_dashboards(...)

I understand why it's made like that, as a single endpoint supports CRUD on two sets of resources, although I wonder if we can abstract that in the library client. For example, we could split the current DashboardList resource into two distinct resources: one for managing dashboard lists, the other for mapping dashboards to dashboard lists.

I don't have a clear solution to suggest. Instead I am happy to discuss this directly with you, to figure something out together.

'POST',
'host/api/v1/actionname/{0}'.format(actionable_object_id),
data={'mydata': "val"}
)
Copy link
Member

Choose a reason for hiding this comment

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

🍰

else:
return APIClient.submit(method, name + "/" + str(id), params)
return APIClient.submit(method, name + "/" + str(id), body)
Copy link
Member

Choose a reason for hiding this comment

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

🍰

Copy link
Member

@yannmh yannmh left a comment

Choose a reason for hiding this comment

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

The changes and the cleanup look great. Thank you!

resource_name=cls._resource_name,
resource_id=id
)
return APIClient.submit('POST', path, body, attach_host_name=attach_host_name, **params)
Copy link
Member

Choose a reason for hiding this comment

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

🍰

@yannmh yannmh merged commit 9e9a146 into master Mar 16, 2018
@yannmh yannmh deleted the marielaure/add_dashboard_list_api branch March 16, 2018 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants