Fix dashboard api call in grafana_dashboard #267
Conversation
|
I found another issue I fixed: When the code was changed, the uid was overwritten by slug (normalized string derived from the title). This breaks existing dashboards by changing their UID (and applications using them). There is also an idempotence issue, I am currently tracking down. The dashboards are being updated every puppet run. I use the JSON export provided by grafana as input but that seems to differ from what the module uses to assert changes. So, before merging this PR, I would like to figure out, what is causing the problem. |
|
The idempotence issue was actually caused by me taking the json from the Export/Share dashboard functionality (or the API call to /dashboards/uid/ without removing the fields "id", "uid", "title" and "version" from it. As the ruby code merges these fields into the hash, the order of fields changes due to the way ruby works. This results in the comparation between current and target to fail as the field order of the two hases is different. I added a sentence to the README for other users to stumble upon it. |
About this. There have been multiple PRs, as far as I saw, in the past trying to address the idempotence issue. There are some measures in places to avoid this (here and here). If you think you can help with that it would be awesome! As far as I see, as a first step in the second link we need to add From your comment, I suppose after merging the two hashes we can sort it as well to ensure the order doesn't change. |
|
I am not that skilled in ruby to actually solve these idempotence issues in the code. I have encountered something similar (when handling yaml files) in https://github.com/voxpupuli/puppet-elasticsearch/blob/master/lib/puppet_x/elastic/hash.rb, but that thing is scary... |
|
Fixed the acceptance test of the folder and set the uid to slug for new dashboards. By setting it to slug, we become predictable and by not assuming its slug, we are backwards compatible |
| @@ -139,7 +139,7 @@ class { 'grafana': | |||
| end | |||
|
|
|||
| it 'folder contains dashboard' do | |||
| shell('curl --user admin:admin http://localhost:3000/api/dashboards/uid/example-dashboard') do |f| | |||
| shell('curl --user admin:admin http://localhost:3000/api/search?query=example-dashboard') do |f| | |||
There was a problem hiding this comment.
Why the search query instead of uid since the uid is now predictable?
There was a problem hiding this comment.
The uid is only predictable when creating new dashboards. Even though I changed the behaviour for new and the old test works for now, I still consider it a more stable solution to not rely on uid=slug. The search works with the name, no matter what is in the uid field.
Pull Request (PR) description
The grafana_dashboard function was changed from using the old deprecated api to the recommended endpoint using the uid. However, the module still passed the dashboard title to the API instead of the uid, always resulting in a dashboard not found error. This PR fixes the problem.
This Pull Request (PR) fixes the following issues
Fixes #261