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

backend: Update DataSourceInstanceSettings to implement interface required for plugin validations #1137

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

papagian
Copy link
Contributor

@papagian papagian commented Nov 8, 2024

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Comment on lines +132 to +135
var dat map[string]interface{}
if err := json.Unmarshal(s.JSONData, &dat); err != nil {
return false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this function will be executed for each query and unmarshalling the whole JSONData (again) would be an expensive operation.

My proposal would be to store a flag (isSecureSocksDSProxyEnabled or similar in the DatasourceInstanceSettings), which can be set when reading these properties in the ProxyOptions function. This function is called when creating the HTTP client in the datasource instantiation.

In any case, @wbrowne may have a better suggestion for this.

Copy link
Member

Choose a reason for hiding this comment

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

I was wondering do we actually need this to be part of DataSourceInstanceSettings and not just datasources.DataSource in Grafana instead IE does this have any practical meaning outside of the Grafana server? If not, could we just validate the datasource model.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how DataSourceInstanceSettings differs from datasources.DataSource but
based on the following snippet I believe when DataSourceInstanceSettings is available we need to use its information for validating the request (instead of using the datasources.DataSource.URL:
https://github.com/grafana/grafana/blob/7eb4b974e0407a126af0a5a1b2b538dd9611d30e/pkg/api/plugin_resource.go#L65-L70

Copy link
Member

Choose a reason for hiding this comment

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

In this case I think it would be okay to just use datasource.URL since that's where the data is originating from https://github.com/grafana/grafana/blob/main/pkg/services/pluginsintegration/adapters/adapters.go#L15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok then, I will close this and I will make the required changes in grafana/grafana
but I admit that I don't have a lot of knowledge to foresee any implications

Copy link
Member

Choose a reason for hiding this comment

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

In case I'm overlooking something, feel free to keep these open as a fallback and try another PR if you prefer! We can help with anything unforeseen ❤️

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