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

Store access token in secureJsonData #277

Merged
merged 2 commits into from
Dec 21, 2024
Merged

Conversation

nineinchnick
Copy link
Member

@nineinchnick nineinchnick commented Nov 18, 2024

Copy link
Member

@jschroth jschroth left a comment

Choose a reason for hiding this comment

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

Thanks for starting this. I tested this with these recommended changes here and it seems to work. The only thing missing is using the secure data:

in settings.go add something like the following before the return statement

	if token, exists := config.DecryptedSecureJSONData["accessToken"]; exists {
		s.AccessToken = token
	}

src/ConfigEditor.tsx Outdated Show resolved Hide resolved
src/ConfigEditor.tsx Outdated Show resolved Hide resolved
@nineinchnick nineinchnick marked this pull request as ready for review November 24, 2024 17:01
Copy link
Member

@jschroth jschroth left a comment

Choose a reason for hiding this comment

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

Not sure that my approval matters, but this works perfectly!

@nineinchnick
Copy link
Member Author

It does, thank you for testing this!

@llamafilm
Copy link

Is there anything we can do to help accelerate releasing this?

@nineinchnick
Copy link
Member Author

You can put pressure on me. Just kidding, but asking for it is enough. As usual, I can't promise anything, but I'll try releasing a new version this week.

@nineinchnick
Copy link
Member Author

I haven't gotten to it yet. The biggest stopper for me is that I have to test the changes manually, and I do this so infrequently, that I lose time remembering how to run a particular version of Grafana locally.
It would speed things up significantly if we had some e2e test workflow, running both Trino and Grafana. I wouldn't have to do manual testing, and it would serve as a reference on how to run things locally, if needed.

@jschroth
Copy link
Member

@nineinchnick I tested the changes from my end and it worked with my setup; although, I was using a production trino instance so it was not something had to setup manually. The grafana piece was just running the plugin locally in a docker container. I wrote up these notes to remind me of what I need to do get the plugin running in docker and then connect to http://localhost:3000.

# get plugin source
git clone https://github.com/trinodb/grafana-trino.git
cd grafana-trino
git fetch origin pull/277/head:pr277
git checkout pr277

# node + build
fnm use 19
yarn install
yarn dev

# mage (run `go mod tidy` if needed)
mage -v

# run
docker run --rm -p 3000:3000 \
  -v "$(pwd):/var/lib/grafana/plugins/trino" \
  -e "GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS=trino-datasource" \
  --name=grafana  grafana/grafana-enterprise

@nineinchnick nineinchnick force-pushed the secure-access-token branch 3 times, most recently from 35d292e to ea0df39 Compare December 21, 2024 20:18
@nineinchnick nineinchnick merged commit 2d07cf0 into main Dec 21, 2024
2 checks passed
@nineinchnick nineinchnick deleted the secure-access-token branch December 21, 2024 20:44
@nineinchnick
Copy link
Member Author

I added E2E tests, released v1.0.10 and submitted it to Grafana for review. Hopefully it'll only take a few days, but I wouldn't be surprised if it'll get reviewed after the new year.

@nineinchnick
Copy link
Member Author

1.0.10 has been published! Thanks everyone for your patience: https://grafana.com/grafana/plugins/trino-datasource/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants