[grafana] Improve dashboard variable substitution

[toanju]

trafficstars

The Grafana dashboard variable syntax allows varnames without curly braces ($datasource and ${datasource}). A substitution can only be applied for the latter. This enables extended regular expressions for sed to allow both substitutions to take place to include dashboards that are written in either form.

[CLAassistant]

All committers have signed the CLA.

[toanju]

I am happy to update this once more, but I'd really appreciate if this time I could have a 2nd review. Many thanks @zanhsieh for already looking into this.

[zanhsieh]

@zalegrala Can you review this plz?

[zalegrala]

Have you had a chance to test this? It seems like we should be matching the closing brace also.

[toanju]

The matches were modified to match braces as optional. Hence, the closing brace will match if it is there, e.g ${DATASOURCE} and $DATASOURCE will match. So far only ${DATASOURCE} was matching.

[zalegrala]

Looking at the old regex, I can test from the CLI with the following.

❯ echo '$variable' | sed '/-- .* --/! s/${{"{"}}{{ .name }}}/{{ .value }}/g'
$variable

With the new regex and the -E flag on sed, I get the following.

❯ echo '$variable' | sed -E '/-- .* --/! s/\$\{{"{"}}?{{ .name }}\}?/{{ .value }}/g'
sed: -e expression #1, char 54: Invalid content of \{\}

How can we validate this before merge?

[toanju]

Please be aware that this is a helm chart and hence first processed by helm. Thus {{"{"}} will replaced with only { as in the original regex*. In addition {{ .name }} and {{ .value }} will be replaced according to the desired substitution.

*) in the new regex this could be omitted since there are no longer 3 { in a row, but I left it to ease comparison with the original regex

[toanju]

Hi @zalegrala, was the above description helpful or do you have any additional questions? In addition, I updated the branch once more.

[toanju]

Yes, this works on Alpine which eventually uses busybox sed. The latter has extended regex expressions (ERE) implemented. Thus, -E should be treated similar on different sed variants supporting ERE.

Though, thinking about the split of the regex made me think regarding the variable naming. One issue that might be related to this having a datasource variable name that is the prefix of another datasource variable, e.g. ds and ds_something.

This could be mitigated by having to sed commands. The first as it was so far and the second without the curly braces but including the quotes.

What do you think?

[jkroepke]

This could be mitigated by having to sed commands. The first as it was so far and the second without the curly braces but including the quotes.

I'm more a fan of practical solution over cool solutions. If there is already an potential use case, that could break and it can be avoided, then we go of this.

Pretty sure there is a 1 in 100 that a dashboard using ds as prefix for something.