search

grafana / sqlds

A package that assists writing SQL-driven datasources
Apache License 2.0
17 stars 12 forks source link

Chore: migrate sqlds to use sqlutil query & macros #105

Closed njvrzm closed 7 months ago

njvrzm commented 7 months ago

This addresses #85, updating sqlds to use functionality that has been migrated to grafana-plugin-sdk-go/data/sqlutil. It introduces an interface change, so should involve a bump to v4.

CLAassistant commented 7 months ago

CLA assistant check
All committers have signed the CLA.

CLAassistant commented 7 months ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

sarahzinger commented 7 months ago

Hiya!

Thanks for diving in :) It's been a minute since I looked at this sqlutils stuff. Is there a document somewhere we can review that outlines why we're moving this to the sdk again? I think it might be good to document in this repo like where the dividing line is between sqlds, the pluginsdk, and other repos.

Where I'm coming from: In my mind anything sql-y would be owned by sqlds and the sdk doesn't need to understand what sql is.

I do think it makes sense to switch to a more util-type of architecture though so rather than define "this is a sqlds" we instead say "this is a backend plugin that calls some sql-focused helpers"

Apologies though I feel like this is late-feedback on my part and I'm probably missing additional context, though I suspect since it sounds like this is a breaking change that impacts other plugin owners as well, other folks might have similar questions?

njvrzm commented 7 months ago

For the record, we discussed this in a call and decided to proceed here - the hope is that eventually sqlds will be entirely subsumed into grafana-plugin-sdk-go. I'm going to update this PR so it's not a breaking change, however, making the removed methods wrappers for those in sqlutil for now, with the plan to deprecate and then remove them later.

njvrzm commented 7 months ago

Closing this in favor of https://github.com/grafana/sqlds/pull/107, which maintains compatibility and is a lot simpler.