Add DeleteGraphDef to graph-defs API #157
Conversation
|
|
||
| // DeleteGraphDef deletes a graph definition. | ||
| func (c *Client) DeleteGraphDef(name string) error { | ||
| _, err := requestJSON[any](c, http.MethodDelete, "/api/v0/graph-defs/delete", map[string]string{"name": name}) |
There was a problem hiding this comment.
Oops, this is the first DELETE API requiring the request body :)
There was a problem hiding this comment.
Yes :) We did so, although it is somewhat unusual, because while it is common to DELETE a resource by including an ID or similar in the URL path, I was hesitant to include a part of the metric name in the path as an ID.
There was a problem hiding this comment.
I think you could use the percent encoded name in path like /api/v0/hosts-by-custom-identifier/{custom-identifier}, but I'm not strongly against using the request body. My another concern is the verb in the paths is unusual in Mackerel APIs. For example POST /api/v0/graph-defs and DELETE /api/v0/graph-defs (or DELETE /api/v0/graph-defs/{metric-name}) are more aligned to other APIs? I remember some (Google?) API design guides recommend POST /{resource-name}/:{verb}, but consistency in the API set is more important anyway. The POST /api/v0/graph-defs/create is an old(est?) API so you'll need to keep the alias forever (GET /api/v0/hosts.json is still there I believe).
There was a problem hiding this comment.
We considered the idea of putting the name parameter in the URL path, but we thought it was unnatural as our internal data model (sorry for the internal talk). We are still puzzled, but we decided to put it in the request body. I hope you will forgive me.
Regarding the second topic, you are absolutely right about the /delete in the URL path. It should be DELETE /api/v0/graph-defs. We will fix it, so please wait.
ref: https://mackerel.io/api-docs/entry/host-metrics#delete-graphdef