search

wildmountainfarms / wild-graphql-datasource

Grafana data source to interpret GraphQL queries as timeseries data. Includes a GraphiQL query editor with autocomplete!
https://grafana.com/grafana/plugins/retrodaredevil-wildgraphql-datasource/
MIT License
4 stars 2 forks source link

Feat: Support for custom headers #5

Closed AndreZiviani closed 3 months ago

AndreZiviani commented 3 months ago

This PR does two things (each on its own commit):

The update process was just running npx @grafana/create-plugin@latest update && go get -u && go mod tidy

AndreZiviani commented 3 months ago

@retrodaredevil not sure if you are accepting PRs but I really want to use this plugin

retrodaredevil commented 3 months ago

I do generally welcome PRs on my projects. I'll take a closer look later and will likely have you change a few things.

Do keep in mind that docker-compose is deprecated in favor of just docker compose, which is built in.

It might be a few days until I get the time to fully review this PR

AndreZiviani commented 3 months ago

I do generally welcome PRs on my projects. I'll take a closer look later and will likely have you change a few things.

cool, no problem

Do keep in mind that docker-compose is deprecated in favor of just docker compose, which is built in.

sure, but it still uses the docker-compose.yml file

It might be a few days until I get the time to fully review this PR

no problem

retrodaredevil commented 3 months ago

I'm used to following 

npm run dev
mage -v build:linux
npm run server

to get up and running. Has that changed since the addition of this delve stuff? The new stuff looks useful for debugging but now I'm unsure of how to actually test these change.

Additionally, I think to get the CI to pass you need to run an npm install so that the lock file updates.

I plan to do a squash merge on these changes. I'm also curious what GraphQL server you are using that requires you to have an Accept header, although it makes sense as the draft of the spec does say the server is allowed to do that https://graphql.github.io/graphql-over-http/draft/#sec-Accept

AndreZiviani commented 3 months ago

Has that changed since the addition of this delve stuff?

no

The new stuff looks useful for debugging but now I'm unsure of how to actually test these change.

you have a few options:

Whenever you make any changes on your code it will automatically rebuild and restart the plugin inside the container (which means you will have to reconnect your delve session)

I'm also curious what GraphQL server you are using that requires you to have an Accept header, although it makes sense as the draft of the spec does say the server is allowed to do that

I don't know what they are using/doing, I'm just an end user

Additionally, I think to get the CI to pass you need to run an npm install so that the lock file updates.

My bad, fixed

AndreZiviani commented 3 months ago

@retrodaredevil friendly bump :grin:

retrodaredevil commented 3 months ago

New job go brrr. Hoping to find some free time in the next few days. Just need to test this locally and give the changes another quick look.

retrodaredevil commented 3 months ago

Looks like to get it to run now I had to do this (discovered this from https://github.com/microsoft/vscode-go/issues/3098#issuecomment-600192657):

echo 0 > /proc/sys/kernel/yama/ptrace_scope
# or if you are not in a root shell:
echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope

That allowed the delve debugger thing to start which happens when the docker container starts up.

  • Update Grafana Plugin SDK in order to use the native custom headers support

So, I'm actually curious what changes for native custom header support have been made that give native custom header support. I'm pretty sure that custom headers were allowed before the update, I just don't think they were secure like I want to eventually add.

In other words, the only thing I notice this PR actually changing is the Accept: application/json header and dependency updates. Is that not the case?

AndreZiviani commented 3 months ago

Looks like to get it to run now I had to do this (discovered this from https://github.com/microsoft/vscode-go/issues/3098#issuecomment-600192657):

echo 0 > /proc/sys/kernel/yama/ptrace_scope or if you are not in a root shell: echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope That allowed the delve debugger thing to start which happens when the docker container starts up.

That's odd, I didn't have this issue, please revert what you did and try the latest commit

So, I'm actually curious what changes for native custom header support have been made that give native custom header support.

I believe this is the PR that made it possible

I'm pretty sure that custom headers were allowed before the update, I just don't think they were secure like I want to eventually add.

They were sent to the pluging but only as a map, you would have to implement the header injection yourself, now the SDK does it for you automaticaly

In other words, the only thing I notice this PR actually changing is the Accept: application/json header and dependency updates. Is that not the case?

That is correct, you can npx @grafana/create-plugin@latest update && go get -u && go mod tidy yourself to verify and the addition of the Accept header is the only actual change made by me

retrodaredevil commented 3 months ago

Very well done! Thanks for the PR! I assume you'd like me to make a release sooner rather than later so you can use the up to date version in your Grafana instance?

AndreZiviani commented 3 months ago

Thanks, yeah that would be awesome

retrodaredevil commented 3 months ago

I believe this is the PR that made it possible

I looked more closely at this, and it looks like it was merged way before grafana-plugin-sdk-go/v0.217.0 was released. So, I'm not sure exactly what upgrading the SDK helped you with, but it probably needed to be done anyway

AndreZiviani commented 2 months ago

het @retrodaredevil is it expected to take this long for this new version to be available on the Grafana Plugins system/page?

retrodaredevil commented 2 months ago

My bad. I submitted a plugin update submission to Grafana just now. I don't know how long plugin updates take, but when I submitted the plugin initially, I got some feedback on changes to make, and then after I made the changes it took maybe about a week for it to get published. So for this small change, I would imagine it will only take about a week, but my upper estimate is two weeks.

AndreZiviani commented 2 months ago

No problem, thanks again

retrodaredevil commented 2 months ago

Update: I had to fix a link in the plugin.json (unrelated to this PR), so I just now fixed that and I expect the plugin update to be live Monday or Tuesday. (I assume the people reviewing the updates only work on weekdays, otherwise it would be sooner)

AndreZiviani commented 2 months ago

hey @retrodaredevil just wanted to say that I've updated the plugin and it is working correctly, thanks!