Add datasources list method #26
Conversation
Change JSONData and SecureJsonData into map[string]interface
| @@ -97,6 +88,33 @@ func (c *Client) UpdateDataSource(s *DataSource) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (c *Client) DataSources() ([]DataSource, error) { | |||
There was a problem hiding this comment.
It would be nice to have a test for this method, and even nicer that it'd have an example of the payload the api returns.
There was a problem hiding this comment.
We will prepare some additional tests.
Regarding payload, for example we use Azure Monitor and Zabbix, both having different jsonData and secureJsopData
Azure Monitor:
{
"id": 101,
"orgId": 1,
"name": "SOME_DATASOURCE_NAME",
"type": "grafana-azure-monitor-datasource",
"typeLogoUrl": "",
"access": "proxy",
"url": "/api/datasources/proxy/101",
"password": "",
"user": "",
"database": "",
"basicAuth": false,
"basicAuthUser": "",
"basicAuthPassword": "",
"withCredentials": false,
"isDefault": false,
"jsonData": {
"appInsightsAppId": "00000000-0000-0000-0000-000000000000",
"azureLogAnalyticsSameAs": false,
"cloudName": "azuremonitor"
},
"secureJsonData": {
"appInsightsApiKey":"bUtGHDSq7XtxsKeBUrfwYfk6QhGWZRP2
},
"version": 5,
"readOnly": false
}
Zabbix:
{
"id": 67,
"orgId": 1,
"name": "Zabbix",
"type": "alexanderzobnin-zabbix-datasource",
"typeLogoUrl": "",
"access": "proxy",
"url": "http://zabbi/api_jsonrpc.php",
"password": "",
"user": "",
"database": "",
"basicAuth": false,
"basicAuthUser": "",
"basicAuthPassword": "",
"withCredentials": false,
"isDefault": false,
"jsonData": {
"addThresholds": true,
"alerting": true,
"alertingMinSeverity": 3,
"dbConnectionDatasourceId": null,
"dbConnectionEnable": false,
"disableReadOnlyUsersAck": false,
"keepCookies": [],
"password": "KdrVK8U0x3hgBppY",
"trends": false,
"username": "grafana",
"zabbixVersion": 4
},
"secureJsonData": {},
"version": 8,
"readOnly": false
}
| DefaultRegion string `json:"defaultRegion,omitempty"` | ||
| TlsSkipVerify bool `json:"tlsSkipVerify,omitempty"` | ||
| } | ||
| type JSONData map[string]interface{} |
There was a problem hiding this comment.
I'm curious to hear your reasoning for changing the types here. Do we have to worry about any potential consumers of Datasource method since they'll have to change how they access the JSONData and SecureJSONData properties?
There was a problem hiding this comment.
There are multiple DataSource types available for Grafana and struct here allows only to use go-grafana-api only with one DataSource (I assume it was something with AWS).
If we loosen typing here to map[string]interface{} we are able to pass different jsonData and secureJsonData needed for other datasource types.
We know breaking changes are not welcomed into the interfaces and we are more than happy to make this possible without breaking changes but I don't see a way how to achieve that. Maybe with reflect or changing type to some Inteface but I am not a fan of either.
There was a problem hiding this comment.
Breaking changes aside, I'm not sure it's necessary to remove the schema from jsonData and secureJsonData entirely to support additional data source types. There tends to be some overlap in values between the different data sources; how we've handled different schemas for similar objects is to merge the maps of all the possible keys of the different data source types and tack on the omitempty tag to most of them so that they won't be sent unless explicitly used. Does that sound like an alright approach to you?
There was a problem hiding this comment.
Ditto to the points that @ghmeier makes, these would be breaking changes for downstream clients, for example in the terraform provider, and I don't necessarily favor that unless there's a compelling reason to.
Add datasources list method