Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

Update doitintl-bigquery-datasource to V2.0.2 #913

Merged
merged 5 commits into from
Jun 25, 2021
Merged

Update doitintl-bigquery-datasource to V2.0.2 #913

merged 5 commits into from
Jun 25, 2021

Conversation

ofir5300
Copy link
Contributor

@ofir5300 ofir5300 commented Apr 4, 2021

Issues solved on V2.0.2:

  • [closed] Required role in introduction text is not correct. #319
  • [closed] Object is not iterable error when result set is empty #316
  • [closed] Partition timestamp WHERE filters convert timezone incorrectly #315
  • [closed] Raw SQL mode fails to extract table name from FROM clause when using variables or when using FROM in a subquery. #314
  • [closed] Using the partition column as timeColumn results in that column appearing multiple times in the generated WHERE clause #309
  • [closed] UTC conversion is returning offset data #307
  • [closed] Occur Query run error when alerting #303
  • [duplicate] Date functions not returning Data Properly. #300
  • [duplicate] A simple Test Case to Validate each release of the Plugin #298
  • [duplicate] Same Query - Different Results from Google Console vs. Grafana-Doit-bigquery plugin #297
  • [closed] How to write subquery without from clause getting interpreted as project name #294

@ofir5300 ofir5300 changed the title Add doitintl-bigquery-datasource v2.0.2 Update doitintl-bigquery-datasource to V2.0.2 Apr 4, 2021
@ofir5300
Copy link
Contributor Author

ofir5300 commented May 3, 2021

@marcusolsson Can you overview this please?

@marcusolsson
Copy link
Contributor

It seems like the packaged plugin contains a __MACOSX directory that makes the plugin fail validation. Could you remove it and repackage the plugin?

@marcusolsson marcusolsson added the type/datasource Categorizes the submission as a data source label May 10, 2021
@ofir5300
Copy link
Contributor Author

ofir5300 commented May 10, 2021

@marcusolsson
I uploaded a zip file which is free of __MACOSX but weirdly I couldn't get an answer from the plugin-validator.

Edit:
I tried validating my plugin using your plugin-validator repository and ran into this issue:
Screen Shot 2021-05-10 at 11 54 33

@marcusolsson
Copy link
Contributor

It reported a (cryptic) error about there has to be only be a single directory in the root of the archive. The ZIP archive must contain a single directory named after the plugin ID that contains the contents of the dist directory:

This should produce the correct package:

mv dist/ doitintl-bigquery-datasource
zip doitintl-bigquery-datasource-2.0.2.zip doitintl-bigquery-datasource -r

For more information on packaging plugins, check out Package a plugin.

@ofir5300
Copy link
Contributor Author

@marcusolsson
Done. Tnx

@marcusolsson
Copy link
Contributor

Looks good! You can follow the review progress on the project board.

@daniellee
Copy link
Contributor

@marcusolsson

Modified signature

I am seeing this when I try to load the plugin:

image

Does the plugin need to resigned after the repackaging that @ofir5300 did? (Question for @marcusolsson )

Plugin checker error

When I run the plugin check on the release zip file, I get an error for the scopes field in the jwtTokenAuth section but I wonder if that is an error in our validation. Looking at the scopes field in our code and shouldn't it be an array of strings and not just a string?

Testing this file:

plugincheck https://github.com/doitintl/bigquery-grafana/releases/download/2.0.2/doitintl-bigquery-datasource-2.0.2.zip

Result:

{
  "level": "error",
  "message": "Invalid plugin.json",
  "details": "`routes.0.jwtTokenAuth.scopes`: Invalid type. Expected: string, given: array\n\nFor more information, refer to the [reference documentation](https://grafana.com/docs/grafana/latest/developers/plugins/metadata/)."
}
{
  "level": "warning",
  "message": "README contains developer jargon",
  "details": "Grafana uses the README within the application to help users understand how to use your plugin. Instructions for building and testing the plugin can be confusing for the end user. You can maintain separate instructions for users and developers by replacing the README in the dist directory with the user documentation."
}

@ofir5300
Copy link
Contributor Author

ofir5300 commented Jun 3, 2021

Hi @daniellee,
I noticed this error of strings array but I looked at our previous approved signed plugins and saw they included the same form of scopes.

@daniellee
Copy link
Contributor

@ofir5300 thanks - I think that is a bug on in the validator.

Do you know anything about the modified signature error?

@daniellee
Copy link
Contributor

@ofir5300

Seeing the following error:

The following files were not included in the signature logger=plugins plugin=doitintl-bigquery-datasource files="[
img/InspectPanel.png img/QueryBuilder.png img/QueryInspector.png 
img/QueryOptions.png img/QueryPrioriy.png img/bigquery_enable_api.png img/bq__grafana_upload_key.png img/bq_grafana_key_uploaded.png img/bq_service_account_choose_role.png img/createserviceaccountbutton.png 
img/grafana-bigquery-demo.gif img/newserviceaccount.png]"

@marcusolsson marcusolsson added submission/update Submission is an update to an existing plugin and removed triage/new-update labels Jun 4, 2021
@daniellee
Copy link
Contributor

@ofir5300 did you add images after signing? Do you need help with this?

@ofir5300
Copy link
Contributor Author

@daniellee @marcusolsson
I did added the images only because I saw an error for it.
According to what I remember I resigned the plugin after adding those images.

What should I do? remove images/resign/something else?
Btw I see no such error on the plugin validator, only the routes.0.jwtTokenAuth.scopes error (which you said its a validator bug).

@daniellee
Copy link
Contributor

@ofir5300 I am using the url: https://github.com/doitintl/bigquery-grafana/releases/download/2.0.2/doitintl-bigquery-datasource-2.0.2.zip

Did the release zip get updated after resigning?

@ofir5300
Copy link
Contributor Author

@daniellee It did not, I am aware that I should always sign the most updated zip.
But again, I am willing to do any required modification in order to move forward, let me know.

@daniellee
Copy link
Contributor

@ofir5300 does the plugin actually work for you? I can't get it to load as Grafana disables the plugin (it sees it as being a tampered plugin). You should see this locally in the Grafana you use for development.

@ofir5300
Copy link
Contributor Author

@daniellee It does seem to have issues suddenly...
I am not sure why, because it worked smoothly 2 weeks ago. Can it be related to the Grafana 8 release?

@daniellee
Copy link
Contributor

@ofir5300 it could be due to the Grafana 8 release as I think the plugin signing code is now stricter about tampering (when the plugin files don't match what is in the manifest). If re-signing the plugin code doesn't work then let me know and I can get one of our backend engineers to have a look.

@ofir5300
Copy link
Contributor Author

@daniellee I tried re-signing many times, than yes I would like that please.

@daniellee
Copy link
Contributor

@ofir5300 when I look at the manifest file then it does not contain any ireferences to mages but they are definitely in the zip file. So I think Grafana is not the problem - it is something to do with the signing.

The plugin is using both webpack for the build and then the plugin toolkit for the signing - so guessing that something is going wrong there. @wbrowne is having a looking at your signing process.

@ofir5300
Copy link
Contributor Author

@ofir5300 @wbrowne
Well yes it seems like the images are not suppose to be there, thus I removed it locally.
But still cannot manage to make it work after signing. Should I upload a newer signed zip?

@wbrowne
Copy link
Member

wbrowne commented Jun 16, 2021

I'm having trouble trying to build the plugin myself due to superquery/superquery-lib not being publicly accessible - but to me it looks like you need to resign as there are images in the plugin folder that are not signed in the MANIFEST.txt.

Should also note that maybe you don't want to package the .DS_Store file as it's not useful

@ofir5300
Copy link
Contributor Author

Ok so I followed @marcusolsson instructions to generate a new zip of a signed plugin.
Please take a look at the updated file and supply with further info regarding my issues because I am having difficulties figuring it out by myself @daniellee @wbrowne

@daniellee
Copy link
Contributor

daniellee commented Jun 17, 2021

@ofir5300 the zip seems to be missing for the 2.0.2 GitHub release. I'm getting a 404 when trying to download it.

@ofir5300
Copy link
Contributor Author

@daniellee I updated the zip file a while ago. Idk if you noticed

@daniellee
Copy link
Contributor

@ofir5300 The signing seems fine now but looks like there are other problems related to the build (wrong permissions on the linux and Windows backend binaries, frontend code not loading). I will investigate a bit deeper and see if I can help with your build script - maybe this mixing of a custom webpack script and the Grafana plugin toolkit is not packaging the way it should.

@daniellee
Copy link
Contributor

Ran into the following issues:

  • The code in the new zip is signed properly but the structure looks strange - the files in the root and there is a dist directory with the same files again.
  • The linux and windows binaries have the wrong permissions (they should be executable like the Darwin binary is)
  • The minified code on the frontend doesn't work. It is complaining about a missing ngInject annotation for the datasource constructor. Really hard to troubleshoot this as superquery/superquery-lib seems to be a private library and makes it hard to build locally. I removed the superquery parts and built the production build (yarn build:prod) and it worked fine for me. Looking at the code, the datasource class has an ngInject annotation: https://github.com/doitintl/bigquery-grafana/blob/master/src/datasource.ts#L185 so I don't understand where this error is coming from. Is the code in the GitHub repository is the same as in the zip file?

I would strongly recommend switching to Grafana toolkit and removing the custom webpack builds to make the packaging step easier. Fixing doitintl/bigquery-grafana#340 would make it easier for us to help you - hard to submit a PR to improve the build process without this getting fixed first.

@ofir5300
Copy link
Contributor Author

ofir5300 commented Jun 22, 2021

  • The code in the new zip is signed properly but the structure looks strange - the files in the root and there is a dist directory with the same files again.

Isnt it how it should be packed? I followed @marcusolsson instructions

  • The linux and windows binaries have the wrong permissions (they should be executable like the Darwin binary is)

chmod +rwx FILE suppose to handle it, am I right?

  • The minified code on the frontend doesn't work. It is complaining about a missing ngInject annotation for the datasource constructor. Really hard to troubleshoot this as superquery/superquery-lib seems to be a private library and makes it hard to build locally. I removed the superquery parts and built the production build (yarn build:prod) and it worked fine for me. Looking at the code, the datasource class has an ngInject annotation: https://github.com/doitintl/bigquery-grafana/blob/master/src/datasource.ts#L185 so I don't understand where this error is coming from. Is the code in the GitHub repository is the same as in the zip file?

I will remove the usage of superquery-lib to keep all the included packages public.
Regarding ngInject issue - I am currently checking it. Weirdly it only emerged recently since we started our correspondence regarding my submission

I would strongly recommend switching to Grafana toolkit and removing the custom webpack builds to make the packaging step easier.

I will pay attention and consider that suggestion, I do not wish to make major refactors & changes to the plugin currently.

@daniellee
Copy link
Contributor

I would strongly recommend switching to Grafana toolkit and removing the custom webpack builds to make the packaging step easier.

If the superquery issue is resolved then we are can send a PR to help you with that.

@ofir5300
Copy link
Contributor Author

@daniellee
I have removed usage of SuperqueryLib, added support for Grafana 8 and merged it all to master
I have re-signed the plugin and uploaded it.
Please re-check.

Copy link
Contributor

@daniellee daniellee left a comment

Choose a reason for hiding this comment

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

The plugin signing and Angular injection error are both resolved now.

LGTM and thanks for sticking with it @ofir5300 and getting this fixed.

@daniellee daniellee merged commit 20c6b66 into grafana:master Jun 25, 2021
daniellee added a commit that referenced this pull request Jun 25, 2021
@daniellee
Copy link
Contributor

@ofir5300 I was bit too fast.

There is a reference to an image for a screenshot: https://github.com/doitintl/bigquery-grafana/blob/master/src/plugin.json#L34 which is not included in the zip. Can you either add the image or remove the screenshot from the plugin.json file.

Then we should be able to publish it. Thanks for your patience

@ofir5300
Copy link
Contributor Author

@daniellee
I want to remove it. This image is originally relevant on the readme file, and I see both plugin page in Grafana and grafana.com are using my readme file, so what is the actual affect of stating a screenshot on plugin.json file?

@ofir5300
Copy link
Contributor Author

@daniellee ?

@wbrowne
Copy link
Member

wbrowne commented Jun 29, 2021

@ofir5300 this should give you a good example
https://grafana.com/grafana/plugins/alexanderzobnin-zabbix-app/

And the corresponding plugin.json is inside the json field here https://grafana.com/api/plugins/alexanderzobnin-zabbix-app

If you wanted a link from your README, then you can just point to a URL like Zabbix does

@daniellee
Copy link
Contributor

@ofir5300 sorry for the slow answer - was offline for the last 2 days. As Will says, screenshots are only used on Grafana.com at the top of the plugins page.

Could you remove the line from the plugin.json file if you don't want a screenshot and re-sign and republish the GitHub release for 2.0.2 again. Then I can publish it to Grafana.com. Thanks again.

@ofir5300
Copy link
Contributor Author

ofir5300 commented Jun 29, 2021

@daniellee Ok I understand now.
I will remove the screenshots for now and probably will add some in the future.

I will open a new PR and mention this one inside so you will notice. Is it ok?

Edit: opened #988

@ofir5300 ofir5300 mentioned this pull request Jun 29, 2021
natoscott pushed a commit to natoscott/grafana-plugin-repository that referenced this pull request Jul 12, 2021
* Add doitintl-bigquery-datasource v2.0.2

* Update v2.0.2 zip file

* Add version to the zip name

* Update md5

* Update md5 after supporting Grafana 8
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
submission/update Submission is an update to an existing plugin type/datasource Categorizes the submission as a data source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants