Simplify configuration of datasources #4

dougszumski · 2018-01-19T15:05:33Z

No description provided.

markgoddard · 2018-01-19T15:29:28Z

example/group_vars/all

+    port: 9200
+    host: monasca-elasticsearch
+    # For elasticsearch DB name
+    grafana_conf_project_id: "some-id"


Should this be just project_id?

markgoddard · 2018-01-19T15:36:25Z

tasks/main.yml

+      "{{ grafana_conf_grafana_datasource_definitions[item.key] }}"
+  register: grafana_conf_enabled_datasources
+  with_dict:
+    "{{ grafana_conf_grafana_datasources }}"


How about building the grafana_conf_enabled_datasources list in one set_fact task?

set_fact:
my_var: []

set_fact:
my_var: "{{ my_var + [item] }}"
with_dict: my_dict

I lied. it's still two. You could initialise the list in defaults if you wanted.

Seems cleaner - thanks. Ready for re-review.

This also fixes an issue where the playbook will fail if no dashboards were found, or specified.

markgoddard · 2018-01-23T10:05:53Z

tasks/main.yml

+    grafana_conf_enabled_datasources:
+      "{{ grafana_conf_enabled_datasources + [grafana_conf_grafana_datasource_definitions[item.key]] }}"
+  with_dict:
+    "{{ grafana_conf_grafana_datasources | default({}, True) }}"


nit: Why not put grafana_conf_grafana_datasources in defaults and remove the default here?

markgoddard · 2018-01-23T10:09:49Z

defaults/main.yml

@@ -23,7 +26,7 @@ grafana_conf_grafana_datasource_definitions:
    "user": "",
    "password": "",
    "type": "monasca-datasource",
-    "url": "http://{{ grafana_conf_monasca_api.host }}:{{ grafana_conf_monasca_api.port }}/",
+    "url": "http://{{ item.value.host }}:{{ item.value.port }}/",


For a small (optional) usability enhancement you could set a task variable, then reference that instead of item.host.

vars: datasource: "{{ item.value }}"

Good suggestion - fixed.

markgoddard · 2018-01-23T10:11:50Z

tasks/main.yml

+
+- name: Clone dashboards
+  git:
+    repo: "{{ grafana_conf_grafana_dashboard_repo.path }}"


The example has this as .repo, which matches the module arguments better.

markgoddard · 2018-01-23T10:14:26Z

tasks/main.yml

+- name: Clone dashboards
+  git:
+    repo: "{{ grafana_conf_grafana_dashboard_repo.path }}"
+    dest: "{{ grafana_conf_grafana_dashboard_repo.checkout_path }}"


Should we also support an optional .version?

markgoddard · 2018-01-23T10:17:49Z

tasks/main.yml

-  with_items: "{{ grafana_conf_available_dashboards['files'] }}"
+  with_items:
+    - "{{ grafana_conf_available_dashboards_git['files'] | default([]) }}"
+    - "{{ grafana_conf_available_dashboards_path['files'] | default([]) }}"


This is now a nested list, does that still work, or should we be using with_items over a concatenated list, or using with_nested?

I believe it works although it would be clearer to explicitly concatenate them. Much to some peoples dislike it flattens the list of lists: ansible/ansible#5913

dougszumski · 2018-01-24T14:26:31Z

Ready for re-review.

markgoddard

Hmm, looks like I forgot to submit my comments.

markgoddard · 2018-01-23T15:11:09Z

tasks/main.yml

@@ -98,13 +101,14 @@
    owner: "{{ ansible_user_id }}"
    group: "{{ ansible_user_id }}"
  become: True
-  when: grafana_conf_grafana_dashboard_repo | bool
+  when: grafana_conf_grafana_dashboard_repo is defined


I think | bool was better - the default means that it will always be defined.

Hmm, the bool filter was causing this and the one below to be skipped. I should go back and investigate exactly how it works.

Yeah it's not always obvious how to check things like this. One way that works fairly well is to set the variable to None in defaults:

my_var:

Then check with may_var is not none. That would require changes in kayobe too though. There's also my_var != {} and my_var | length > 0.

Thanks - it should be more robust now. I've checked it behaves with Kayobe defaults, with a repo set, and with some missing repo information.

markgoddard · 2018-01-23T15:11:31Z

tasks/main.yml

    dest: "{{ grafana_conf_grafana_dashboard_repo.checkout_path }}"
-  when: grafana_conf_grafana_dashboard_repo | bool
+    version: "{{ grafana_conf_grafana_dashboard_repo.version | default('HEAD') }}"
+  when: grafana_conf_grafana_dashboard_repo is defined


markgoddard · 2018-01-24T18:07:31Z

tasks/main.yml

+    grafana_conf_dashboard_repo_set: "{{
+      (grafana_conf_grafana_dashboard_repo.repo is not none) and
+      (grafana_conf_grafana_dashboard_repo.checkout_path is not none) and
+      (grafana_conf_grafana_dashboard_repo.relative_path is not none) }}"


Doesn't this break with the role default, an empty dict?

Argh :( Thanks!

dougszumski · 2018-01-29T13:10:42Z

Ready for review.

Simplify configuration of datasources

8e74e25

dougszumski requested a review from markgoddard January 19, 2018 15:05

markgoddard reviewed Jan 19, 2018

View reviewed changes

dougszumski added 3 commits January 19, 2018 17:52

Handle the case when no datasources are set

3761d3a

Address review comment

295f4d5

Support configuring dashboards from a git repo

20f54aa

This also fixes an issue where the playbook will fail if no dashboards were found, or specified.

dougszumski force-pushed the feature/simplify_datasource_config branch from 1185513 to 20f54aa Compare January 22, 2018 16:16

markgoddard requested changes Jan 23, 2018

View reviewed changes

dougszumski added 5 commits January 23, 2018 15:04

Improve naming of datasource variable

0dfb0cc

Move grafana_conf_grafana_datasources default to defaults

b7394db

Match naming in example

595ca42

Support specifying a DB repo version

e7b15d1

Fix issue where dashboards were not being cloned

35268c1

markgoddard requested changes Jan 24, 2018

View reviewed changes

dougszumski force-pushed the feature/simplify_datasource_config branch from 71ede9f to d78e6b4 Compare January 24, 2018 17:49

markgoddard reviewed Jan 24, 2018

View reviewed changes

Group dashboard tasks and validate repo dict

658907e

dougszumski force-pushed the feature/simplify_datasource_config branch from d78e6b4 to 658907e Compare January 24, 2018 21:35

markgoddard approved these changes Jan 29, 2018

View reviewed changes

dougszumski merged commit 66224ef into master Jan 29, 2018

dougszumski deleted the feature/simplify_datasource_config branch January 29, 2018 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify configuration of datasources #4

Simplify configuration of datasources #4

dougszumski commented Jan 19, 2018

markgoddard Jan 19, 2018

markgoddard Jan 19, 2018

dougszumski Jan 22, 2018

markgoddard Jan 23, 2018

dougszumski Jan 23, 2018

markgoddard Jan 23, 2018

dougszumski Jan 23, 2018

markgoddard Jan 23, 2018

dougszumski Jan 23, 2018

markgoddard Jan 23, 2018

dougszumski Jan 23, 2018

markgoddard Jan 23, 2018

dougszumski Jan 23, 2018

dougszumski commented Jan 24, 2018

markgoddard left a comment

markgoddard Jan 23, 2018

dougszumski Jan 24, 2018

markgoddard Jan 24, 2018

dougszumski Jan 24, 2018

markgoddard Jan 23, 2018

markgoddard Jan 24, 2018

dougszumski Jan 24, 2018

dougszumski commented Jan 29, 2018

Simplify configuration of datasources #4

Simplify configuration of datasources #4

Conversation

dougszumski commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougszumski commented Jan 24, 2018

markgoddard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougszumski commented Jan 29, 2018