make GetDBConnectionFromQuery public #71
Conversation
datasource.go
Outdated
| @@ -138,36 +138,36 @@ func (ds *SQLDatasource) QueryData(ctx context.Context, req *backend.QueryDataRe | |||
|
|
|||
| } | |||
|
|
|||
| func (ds *SQLDatasource) getDBConnectionFromQuery(q *Query, datasourceUID string) (string, dbConnection, error) { | |||
| func (ds *SQLDatasource) GetDBConnectionFromQuery(q *Query, datasourceUID string) (string, *sql.DB, *backend.DataSourceInstanceSettings, error) { | |||
There was a problem hiding this comment.
mmh, from the usage I see here, you are only interested on getting the *sql.DB, not the internal key and the instance settings. I would create two different functions here: GetDBFromQuery that returns what you want and getSettings that returns the rest
There was a problem hiding this comment.
I disagree with that a bit, since the logic for getting the *sql.DB and the instance settings is so intertwined that separate GetDBFromQuery and getSettings functions would be basically repeated code. I could see the logic in making GetDBConnectionFromQuery private again and adding GetDBFromQuery that wraps it and just returns the *sql.DB if we want to reduce the exposed information.
There was a problem hiding this comment.
yes, I am not saying that we should repeat code, I am mostly worried that you will be exposing this method with unnecessary things and once you expose it, any change to it would be a breaking change.
I have not taken a deep look to this code so up to you for the final solution, you can either do that wrapper or move common stuff to a different function so you call it from both functions, the external and the internal.
I merged the other commit before realizing that there were other comments on grafana-aws-sdk that related to this package.
Fixes #66