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

Feat: Support for custom headers #5

Merged

Conversation

AndreZiviani
Copy link
Contributor

@AndreZiviani AndreZiviani commented Jun 12, 2024

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

  • Update Grafana Plugin SDK in order to use the native custom headers support
  • Add Accept: application/json header to every request, not sure if it is required on every case but I had to add it in order to work for me
  • Auto rebuild and automatically run delve server inside the container (both from SDK update)

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

@AndreZiviani
Copy link
Contributor Author

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

@retrodaredevil
Copy link
Member

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
Copy link
Contributor Author

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

package.json Outdated Show resolved Hide resolved
@retrodaredevil
Copy link
Member

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
Copy link
Contributor Author

AndreZiviani commented Jun 15, 2024

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:

  • local delve: run dlv connect 127.0.0.1:2345 but delve won't be able to access the source code since it tries to access it on /root/wild-graphql-datasource (which is the path inside the container) because the binary is actually built inside the container
  • remote delve: I prefer to run it inside the container, this way it can show the source code when debugging. Run docker compose exec grafana /root/go/bin/dlv connect 127.0.0.1:2345
  • vscode: haven't actually tried it but I guess it is pretty straightforward (example)

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
Copy link
Contributor Author

@retrodaredevil friendly bump 😁

@retrodaredevil
Copy link
Member

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
Copy link
Member

Looks like to get it to run now I had to do this (discovered this from microsoft/vscode-go#3098 (comment)):

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
Copy link
Contributor Author

Looks like to get it to run now I had to do this (discovered this from microsoft/vscode-go#3098 (comment)):

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 retrodaredevil merged commit 1c883d3 into wildmountainfarms:main Jun 28, 2024
2 checks passed
@retrodaredevil
Copy link
Member

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
Copy link
Contributor Author

Thanks, yeah that would be awesome

@retrodaredevil
Copy link
Member

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
Copy link
Contributor Author

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

@retrodaredevil
Copy link
Member

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 AndreZiviani deleted the feat-custom-headers branch July 9, 2024 14:17
@AndreZiviani
Copy link
Contributor Author

No problem, thanks again

@retrodaredevil
Copy link
Member

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
Copy link
Contributor Author

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

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.

2 participants