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

Dashboard: Support additional variable format options (singlequote, doublequote, sqlstring) #21622

Merged
merged 12 commits into from
Mar 24, 2020
Merged

Conversation

xiaobeiyang
Copy link
Contributor

@xiaobeiyang xiaobeiyang commented Jan 20, 2020

What this PR does / why we need it:
This PR adds 3 formatting options of variables, mainly for SQL.
New formatting list:
singlequote: Quote variable value(s) with single quote, escape single quotes in original value with backslashes.
doublequote: Quote variable value(s) with double quote, escape double quotes in original value with backslashes.
sqlstring: Quote variable value(s) with single quote, escape ' in original value by ''.

When writing SQL statements, we usually need this kind of statement “select * from table1 where name in (${var})”, but Grafana doesn’t provides formatting option to quote string of SQL.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@aknuds1 aknuds1 added area/frontend needs more info Issue needs more information, like query results, dashboard or panel json, grafana version etc labels Jan 20, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Jan 20, 2020

Could you please fill in the section 'What this PR does / why we need it' in the PR template?

@marefr marefr added the pr/external This PR is from external contributor label Jan 20, 2020
@xiaobeiyang
Copy link
Contributor Author

@aknuds1 Thanks for your quick response, PR template has been update.

@torkelo
Copy link
Member

torkelo commented Jan 21, 2020

There should be custom format functions in the Postgres and MySQL datasources I’m that does this I think

@xiaobeiyang
Copy link
Contributor Author

Thanks @torkelo . It is not easy to decide which variable should be quoted/escaped in datasources plugins. For example: select $var1, $var2 from $tableVar where col1=$colVar1 AND col2 in $colVar2 .
I'm writing plugin for other database service instead of Postgres and MySQL. It will be great if Grafana platform provides these formats out of the box, then datasource plugins could have more common behaviors both in developing & using.

docs/sources/reference/templating.md Outdated Show resolved Hide resolved
docs/sources/reference/templating.md Outdated Show resolved Hide resolved
docs/sources/reference/templating.md Outdated Show resolved Hide resolved
Copy link
Member

@ryantxu ryantxu left a comment

Choose a reason for hiding this comment

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

It is nice to have examples in the docs... but I would be waaaay happier with examples in tests :)

@xiaobeiyang
Copy link
Contributor Author

@ryantxu Please help review. Thanks.

Copy link
Contributor Author

@xiaobeiyang xiaobeiyang left a comment

Choose a reason for hiding this comment

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

updated

@xiaobeiyang
Copy link
Contributor Author

Hi @torkelo , is there any other problem in the PR or some process? We expect this RP will be merged and start use it in my dashboards. Please help review, I will fix if there is any problem.
Thanks.

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Really sorry for slow feedback, this looks great now. Just a small detail missing in the docs

docs/sources/reference/templating.md Outdated Show resolved Hide resolved
docs/sources/reference/templating.md Outdated Show resolved Hide resolved
@xiaobeiyang
Copy link
Contributor Author

@torkelo @ryantxu @oddlittlebird Please help review, many thanks.

@oddlittlebird
Copy link
Contributor

LGTM!

Copy link
Contributor Author

@xiaobeiyang xiaobeiyang left a comment

Choose a reason for hiding this comment

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

please help review

Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

Please see my suggestions.

public/app/features/templating/specs/template_srv.test.ts Outdated Show resolved Hide resolved
public/app/features/templating/specs/template_srv.test.ts Outdated Show resolved Hide resolved
public/app/features/templating/specs/template_srv.test.ts Outdated Show resolved Hide resolved
public/app/features/templating/specs/template_srv.test.ts Outdated Show resolved Hide resolved
public/app/features/templating/specs/template_srv.test.ts Outdated Show resolved Hide resolved
public/app/features/templating/template_srv.ts Outdated Show resolved Hide resolved
public/app/features/templating/template_srv.ts Outdated Show resolved Hide resolved
public/app/features/templating/template_srv.ts Outdated Show resolved Hide resolved
public/app/features/templating/template_srv.ts Outdated Show resolved Hide resolved
public/app/features/templating/template_srv.ts Outdated Show resolved Hide resolved
@aknuds1
Copy link
Contributor

aknuds1 commented Feb 26, 2020

Hang on, going to adjust my suggestions as I hear that double quotes are allowed (I'm used to them not being allowed).

@aknuds1
Copy link
Contributor

aknuds1 commented Feb 26, 2020

OK, edited my suggestions :)

@xiaobeiyang xiaobeiyang requested a review from aknuds1 March 3, 2020 07:27
@xiaobeiyang
Copy link
Contributor Author

Thanks @aknuds1 . Please help review again.
The CI grafana-docker-ubuntu-pr step failed due to Ubuntu repository issue. I don't have rerun privilege. It looks like grafana-docker-ubuntu-pr fail didn't cause by the new commits.

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@aknuds1 aknuds1 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 applying my suggestions, I just saw a remaining typo.

public/app/features/templating/template_srv.ts Outdated Show resolved Hide resolved
@aknuds1
Copy link
Contributor

aknuds1 commented Mar 9, 2020

Can you please merge this branch with Grafana master, in order to fix a CI failure?

xiaobeiyang and others added 11 commits March 14, 2020 12:27
Co-Authored-By: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-Authored-By: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-Authored-By: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Update Singlequote and Doublequote descriptions in templating.md
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM

@aknuds1 aknuds1 added add to changelog and removed needs more info Issue needs more information, like query results, dashboard or panel json, grafana version etc labels Mar 14, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Mar 14, 2020

@torkelo @marefr This is good to merge right? Shall we add a milestone?

@aknuds1
Copy link
Contributor

aknuds1 commented Mar 24, 2020

I'm just pushing a merge with latest Grafana master to your branch, in order to fix CI. Hope you don't mind :)

@aknuds1 aknuds1 merged commit 803f553 into grafana:master Mar 24, 2020
@aknuds1
Copy link
Contributor

aknuds1 commented Mar 24, 2020

Thanks very much for contributing to Grafana!

@aknuds1 aknuds1 added this to the 7.0 milestone Mar 24, 2020
@marefr marefr changed the title Variable: Support more variable formatting. Dashboard: Support more variable format options Apr 28, 2020
@marefr marefr changed the title Dashboard: Support more variable format options Dashboard: Support additional variable format options (singlequote, doublequote, sqlstring) Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants