Improve parsing of configuration variables #449
Conversation
aangelisc
requested review from
asimpson and
andresmgot
and removed request for
a team
|
Backend code coverage report for PR #449
|
|
Frontend code coverage report for PR #449 |
pkg/azuredx/models/settings.go
Outdated
| d.CacheMaxAge = jsonData["cacheMaxAge"].(string) | ||
| } | ||
| if jsonData["dynamicCaching"] != nil { | ||
| if dynamicCaching, err := strconv.ParseBool(jsonData["dynamicCaching"].(string)); err == nil { |
There was a problem hiding this comment.
mmh, wouldn't this break if dynamicCaching is actually a boolean? which is the right thing to be, not a string
As far as I can see, from the original issue, the problem is that the user is trying to get boolean values from env variables, which are automatically translated to strings. I believe that can be solved simply casting the value in the YAML:
jsonData:
clusterUrl: ...
tenantId: ...
clientId: ...
onBehalfOf: !!bool $ADX_ON_BEHALF_OF
oauthPassThru: !!bool $ADX_OAUTH_PASS_THRUThere was a problem hiding this comment.
I've realised I originally had a different condition, let me switch this back and it should be safer.
Your suggested solution leads to the following Grafana error:
Datasource provisioning error: yaml: cannot decode !!str `$ADX_ON_BEHALF_OF` as a !!bool
I think casting and checking each type is probably a safer implementation that could avoid issues like these, wdyt?
There was a problem hiding this comment.
mmh, I see, so the problem is that the system that parses the YAML receives the file unchanged so onBehalfOf is actually a string "$ADX_ON_BEHALF_OF" but at some point it gets replaced and the JSON data that the plugin receives is onBehalfOf: "true". Is that correct?
I think this is a bug that should be solved in Grafana, not in the plugin. I see some similar report grafana/grafana#32061 but didn't get any attention. It's not something scalable if we need to parse field-by-field to handle this situation. Do you mind creating an issue (or reopening that one) giving more information of how to reproduce it?
There was a problem hiding this comment.
Yep that's exactly correct, I'll reopen that issue with a reference to this one detailing how to recreate 😊
|
Fixed upstream in grafana/grafana#63085 |
When instantiating a new datasource the current flow assumes that all input variables in the config
JSONDataare of the correct type. This leads to the creation of the datasource failing if an environment variable is used to provide a value for a property that is of typebool.Fixes #442