-
Notifications
You must be signed in to change notification settings - Fork 26
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
Create plugin: CI workflow improvements #890
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
{{#if_eq orgName "grafana"}} | ||
id-token: write | ||
{{/if_eq}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we want to introduce Grafana specific code to create-plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am kind of leaning towards what Jack says, and try to keep this more generic (not introducing internal plugin development related knowledge in the templates).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I can see your worries. It is easy it becomes a slippery slope with lots of Grafana internal specific logic added to the template. On the other hand, we are creating lots of pluggins internally so it might be worth having some of this stuff added via the template. I'm a bit torn 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so what is the alternative? Another separate workflow for internal devs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep my biggest concern here isn't so much this single addition but that we open the door for a community tool to become peppered with our own internal teams "needs". I would vote that we create separate workflows for our internal teams. From what I've gathered they each have their own ideas and would be good to pool those folk together to create something that they can share internally. I'm pretty sure most of them edit/delete this workflow anyhow.
Additionally this template variable is based on user input. If I want my plugin to be called grafana-awesome-panel
I might (through trial and error) come to the conclusion that to do that I need to use grafana
as the org name when I scaffoled. Eventually I submit my plugin for review and I'm asked to change the org name unaware that my plugin was scaffolded with specifics used by Grafanas own plugin teams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree adding Grafana specific code here is not ideal. I'm happy to move this to Braintank or wiki.
|
||
{{#if_eq orgName "grafana"}} | ||
# Repos in the grafana Github org can publish the report to GCS so that they can be browsed without having to download them. | ||
# This will make the report public on the Internet, so beware to skip this step if the report contains sensitive information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What sort of sensitive information could this expose? Could having it as a default be rather risky?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, I'm starting to think that a better approach would be to upload artifacts to github instead (if we want to enable this functionality). Then it would work for all users regarding if it is within the grafana org or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uploading to github would not give you a possibility to link to it - you would have to download the report artifact in a zip file and run npx playwright show-report
uploading to GCS gives you a way to share a link with someone who doesnt want to download unzip and run report locally - so kind of a convenience feature.
Example: grafana/grafana#86835 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What sort of sensitive information could this expose? Could having it as a default be rather risky?
Same as any e2e test report more or less - it contains your browser state, screenshots of the execution, etc. and if you have something sensitive in there it will be in the report. Since it is quite impossible to automatically know about what could theoretically be sensitive on the screen there is no automated solution to this - microsoft/playwright#19992
Hence the guidance in the comments above the action - if you are for some reason using real credentials or have sensitive information in the flow - you have to handle it yourself or not publish it to a public bucket. For most of the plugin devs in grafana org that are just testing with mocks and dummy data it is absolutely fine and even more convenient to share reports via GCS Bucket / URL (same as we do in grafana/grafana).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, and the same security concern applies when uploading artifacts to GH as the report becomes public on the Internet. Playwright docs have example workflows where reports are being uploaded to GH artifacts, bit they don't mention security concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm rather safe than sorry - let's comment out the upload artifacts step.
{{#if_eq orgName "grafana"}} | ||
id-token: write | ||
{{/if_eq}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am kind of leaning towards what Jack says, and try to keep this more generic (not introducing internal plugin development related knowledge in the templates).
Thanks for the review folks. Have removed the grafana specific bits and commented out the report uploading step (see this commit). Let me know what you think. |
🚀 PR was released in |
What this PR does / why we need it:
This PR is making a few changes to the e2e related workflow bits:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
📦 Published PR as canary version:
Canary Versions
✨ Test out this PR locally via:
npm install @grafana/create-plugin@4.7.1-canary.890.babb00f.0 npm install @grafana/plugin-e2e@1.1.2-canary.890.babb00f.0 # or yarn add @grafana/create-plugin@4.7.1-canary.890.babb00f.0 yarn add @grafana/plugin-e2e@1.1.2-canary.890.babb00f.0