add integration installation changed_when reporting #403

camjay · 2021-11-15T23:18:37Z

check for existing version of integrations, don't report changes if integration(s) install is unchanged
add quotes and re-order modified task params for consistency
fix spelling errors

bkabrda · 2021-11-24T13:17:50Z

Hi! Thanks for sending the PR. While this seems to work for now, it might break if the output string changed or became localized. I think it would be better to first run a loop with agent integration show <name> -q which will fail (if the integration is not present) or echo current version (if it is present). Then we could only install integrations for which show failed or returned a version different than what is set in the playbook.

Does that sound reasonable? I can look into it if you don't have time to modify this PR.

add quotes and re-order task params for consistency fix spelling errors

camjay · 2021-12-31T00:05:32Z

@bkabrda I've modified it based on your feedback. Let me know if the spelling fixes should be split out into a separate PR.

bkabrda

Hey 👋 I needed to do quite a lot of changes to make this work properly on both Linux and Windows. I left bunch of suggestions/comments inline, but if you don't want to go through the review and do the changes one by one, here's a full diff that I have locally that seems to work:

diff --git a/defaults/main.yml b/defaults/main.yml
index 30f9f47..29795ff 100644
--- a/defaults/main.yml
+++ b/defaults/main.yml
@@ -186,5 +186,3 @@ datadog_agent7_yum_repo: "https://yum.datadoghq.com/stable/7/{{ ansible_facts.ar
 datadog_agent5_zypper_repo: "https://yum.datadoghq.com/suse/rpm/{{ ansible_facts.architecture }}"
 datadog_agent6_zypper_repo: "https://yum.datadoghq.com/suse/stable/6/{{ ansible_facts.architecture }}"
 datadog_agent7_zypper_repo: "https://yum.datadoghq.com/suse/stable/7/{{ ansible_facts.architecture }}"
-
-integration_results: '{{  integration_existing.results |  json_query ("[?integration.key==integration.key]" ) | first }}'
diff --git a/tasks/integration.yml b/tasks/integration.yml
index 7eaa6ce..20be86b 100644
--- a/tasks/integration.yml
+++ b/tasks/integration.yml
@@ -60,7 +60,7 @@
   become_user: "{{ integration_command_user }}"
   check_mode: no
   changed_when: >-
-    integration_results.rc != 0 or integration_results.stdout != integration.value.version
+    integration_existing.rc != 0 or integration_existing.stdout != integration.value.version

   # Ignore command failure if integration is not already installed
   failed_when: no
@@ -69,42 +69,32 @@
   loop_control:
     loop_var: 'integration'
   when:
-    - "integration.value.action | lower == 'install'"
     - "ansible_facts.os_family != 'Windows'"
-
-
+    - "integration.value.action | lower == 'install'"

 - name: "Check for existing install / version of integrations (Windows)"
-  win_command: '"{{ datadog_agent_binary_path }}" integration show {{ item.key }}'
+  win_command: '"{{ datadog_agent_binary_path }}" integration show {{ integration.key }} -q'
   become: yes
   become_user: "{{ integration_command_user }}"
   check_mode: no
   changed_when: >-
     integration_windows_existing.results.stderr is defined or ( integration_windows_existing.results.stdout is defined
-    and integration_windows_existing.results.stdout is not search ( item.value.version ) )
+    and integration_windows_existing.results.stdout is not search ( integration.value.version ) )

   # Ignore command failure if integration is not already installed
   failed_when: no
   loop: "{{ datadog_integration|dict2items }}"
+  loop_control:
+    loop_var: 'integration'
   register: "integration_windows_existing"
   when:
     - "ansible_facts.os_family == 'Windows'"
-    - "item.value.action | lower == 'install'"
-
-- name: 'debug integration check'
-  debug:
-    var: 'integration_existing'
+    - "integration.value.action | lower == 'install'"

 # Install integrations

 - name: "Install pinned version of integrations (Unix)"
-  command:
-    argv:
-      - "{{ datadog_agent_binary_path }}"
-      - "integration"
-      - "install"
-      - "{{ third_party }}"
-      - "{{ check_result.integration.key }}=={{ check_result.integration.value.version }}"
+  command: "{{ datadog_agent_binary_path }} integration install {{ third_party }} {{ check_result.integration.key }}=={{ check_resul
t.integration.value.version }}"
   become: yes
   become_user: "{{ integration_command_user }}"
@@ -113,24 +103,26 @@
     third_party: "{% if 'third_party' in check_result.integration.value and check_result.integration.value.third_party | bool %}--th
ird-party{% endif %}"
   loop: "{{ integration_existing.results  }}"
   loop_control:
-    loop_var: 'check_result'
+    loop_var: "check_result"
   register: "integration_unix_install"
   when:
+    - "ansible_facts.os_family != 'Windows'"
     - "check_result.integration.value.action | lower == 'install'"
     - "check_result.rc != 0 or check_result.stdout is undefined or check_result.stdout != check_result.integration.value.version"
-    - "ansible_facts.os_family != 'Windows'"

 - name: "Install pinned version of integrations (Windows)"
-  win_command: '"{{ datadog_agent_binary_path }}" integration install {{ third_party }} {{ item.key }}=={{ item.value.version }}'
+  win_command: '"{{ datadog_agent_binary_path }}" integration install {{ third_party }} {{ check_result.integration.key }}=={{ check
_result.integration.value.version }}'
   become: yes
   become_user: "{{ integration_command_user }}"

-  changed_when: "integration_windows_existing.results.stdout is undefined or integration_windows_existing.results.stdout is not sear
ch ( item.value.version )"
-  loop: "{{ datadog_integration|dict2items }}"
-  register: "integration_windows_install"
+  changed_when: "check_result.stdout is undefined or check_result.stdout is not search ( check_result.integration.value.version )"
   vars:
-    third_party: "{% if 'third_party' in item.value and item.value.third_party | bool %}--third-party{% endif %}"
+    third_party: "{% if 'third_party' in check_result.integration.value and check_result.integration.value.third_party | bool %}--th
ird-party{% endif %}"
+  loop: "{{ integration_windows_existing.results }}"
+  loop_control:
+    loop_var: "check_result"
+  register: "integration_windows_install"
   when:
-    - "item.value.action | lower== 'install'"
     - "ansible_facts.os_family == 'Windows'"
-    - "integration_windows_existing.results.stdout is undefined or integration_windows_existing.results.stdout is not search ( item.
value.version )"
+    - "check_result.integration.value.action | lower == 'install'"
+    - "check_result.stdout is undefined or check_result.stdout is not search ( check_result.integration.value.version )"

bkabrda · 2022-01-26T12:05:23Z

defaults/main.yml

@@ -182,3 +182,5 @@ datadog_agent7_yum_repo: "https://yum.datadoghq.com/stable/7/{{ ansible_facts.ar
 datadog_agent5_zypper_repo: "https://yum.datadoghq.com/suse/rpm/{{ ansible_facts.architecture }}"
 datadog_agent6_zypper_repo: "https://yum.datadoghq.com/suse/stable/6/{{ ansible_facts.architecture }}"
 datadog_agent7_zypper_repo: "https://yum.datadoghq.com/suse/stable/7/{{ ansible_facts.architecture }}"
+
+integration_results: '{{ integration_existing.results | json_query ("[?integration.key==integration.key]" ) | first }}'


I think this isn't necessary at all (at least I got your PR working and excluded this, see below).

bkabrda · 2022-01-26T12:06:03Z

tasks/integration.yml

- third_party: "{% if 'third_party' in item.value and item.value.third_party | bool %}--third-party{% endif %}"
+ check_mode: no
+ changed_when: >-
+ integration_results.rc != 0 or integration_results.stdout != integration.value.version


Suggested change

integration_results.rc != 0 or integration_results.stdout != integration.value.version

integration_existing.rc != 0 or integration_existing.stdout != integration.value.version

bkabrda · 2022-01-26T12:07:50Z

tasks/integration.yml

+ loop_var: 'integration'
+ when:
+ - "integration.value.action | lower == 'install'"
+ - "ansible_facts.os_family != 'Windows'"


I prefer to keep the platform checks as the first condition - while at this case it doesn't matter, there is a place in this file where it does matter and it's good to keep things consistent.

Also, please remove the two blank lines below.

bkabrda · 2022-01-26T12:08:17Z

tasks/integration.yml

+
+
+- name: "Check for existing install / version of integrations (Windows)"
+ win_command: '"{{ datadog_agent_binary_path }}" integration show {{ item.key }}'


Suggested change

win_command: '"{{ datadog_agent_binary_path }}" integration show {{ item.key }}'

win_command: '"{{ datadog_agent_binary_path }}" integration show {{ integration.key }} -q'

bkabrda · 2022-01-26T12:09:27Z

tasks/integration.yml

+ check_mode: no
+ changed_when: >-
+ integration_windows_existing.results.stderr is defined or ( integration_windows_existing.results.stdout is defined
+ and integration_windows_existing.results.stdout is not search ( item.value.version ) )


Suggested change

and integration_windows_existing.results.stdout is not search ( item.value.version ) )

and integration_windows_existing.results.stdout is not search ( integration.value.version ) )

bkabrda · 2022-01-26T12:10:30Z

tasks/integration.yml

 loop: "{{ datadog_integration|dict2items }}"
- when: item.value.action == "install" and ansible_facts.os_family != "Windows"
+ register: "integration_windows_existing"


It would be great to also use integration loop var here to keep things consistent with the 'Unix' steps.

bkabrda · 2022-01-26T12:12:15Z

tasks/integration.yml

+ - "{{ datadog_agent_binary_path }}"
+ - "integration"
+ - "install"
+ - "{{ third_party }}"


When third_party is empty, this makes the command fail (because it will be passed as an empty string argument). You need to use the one-line version.

bkabrda · 2022-01-26T12:12:47Z

tasks/integration.yml

+ when:
+ - "check_result.integration.value.action | lower == 'install'"
+ - "check_result.rc != 0 or check_result.stdout is undefined or check_result.stdout != check_result.integration.value.version"
+ - "ansible_facts.os_family != 'Windows'"


Again, let's move the platform check to the top.

bkabrda · 2022-01-26T12:13:48Z

tasks/integration.yml

+ - "check_result.rc != 0 or check_result.stdout is undefined or check_result.stdout != check_result.integration.value.version"
+ - "ansible_facts.os_family != 'Windows'"
+
+- name: "Install pinned version of integrations (Windows)"


It'd be much better to do the same as we do for Unix, to keep things consistent. A full example of what I'd do is in the PR review summary.

add changed_when reporting

d4928b1

camjay requested a review from a team as a code owner November 15, 2021 23:18

camjay changed the title ~~add changed_when reporting~~ add integration installation changed_when reporting Nov 15, 2021

camjay marked this pull request as draft December 29, 2021 19:28

camjay force-pushed the cp.integration_changed branch 5 times, most recently from b14b67a to c93ce1a Compare December 30, 2021 23:50

check for existing version of integrations

ccf7c37

add quotes and re-order task params for consistency fix spelling errors

camjay force-pushed the cp.integration_changed branch from dab1b89 to ccf7c37 Compare December 31, 2021 00:00

camjay marked this pull request as ready for review December 31, 2021 00:04

camjay marked this pull request as draft January 3, 2022 16:39

is not

56ca38a

camjay force-pushed the cp.integration_changed branch from 0bd8a66 to 56ca38a Compare January 3, 2022 17:01

camjay added 13 commits January 3, 2022 10:09

add debug

c547291

results

93f4b25

loop over results

84f4938

changed if error

a5911db

report chanfe

ad2baa9

json query

828bd38

brackets

4d8b806

add debug

19d6d3c

filter name

83de8cc

add pipe

848ddb5

cleanup

5be8a40

improve debug

778d045

correct var name

da6dcda

camjay added 9 commits January 3, 2022 13:50

var not text

e1dc356

correct function name

ba1ac33

simplify

0dc1fec

update var names

fd7b3a3

report change

d5a80ce

add def check

1f98f50

change reporting

08e61c0

first result that matches

df8c269

no braces

a67c40b

bkabrda suggested changes Jan 26, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add integration installation changed_when reporting #403

add integration installation changed_when reporting #403

camjay commented Nov 15, 2021 •

edited

bkabrda commented Nov 24, 2021

camjay commented Dec 31, 2021

bkabrda left a comment

bkabrda Jan 26, 2022

bkabrda Jan 26, 2022

bkabrda Jan 26, 2022

bkabrda Jan 26, 2022

bkabrda Jan 26, 2022

bkabrda Jan 26, 2022

bkabrda Jan 26, 2022

bkabrda Jan 26, 2022

bkabrda Jan 26, 2022

	integration_results.rc != 0 or integration_results.stdout != integration.value.version
	integration_existing.rc != 0 or integration_existing.stdout != integration.value.version



		- name: "Check for existing install / version of integrations (Windows)"
		win_command: '"{{ datadog_agent_binary_path }}" integration show {{ item.key }}'

	and integration_windows_existing.results.stdout is not search ( item.value.version ) )
	and integration_windows_existing.results.stdout is not search ( integration.value.version ) )

add integration installation changed_when reporting #403

Are you sure you want to change the base?

add integration installation changed_when reporting #403

Conversation

camjay commented Nov 15, 2021 • edited

bkabrda commented Nov 24, 2021

camjay commented Dec 31, 2021

bkabrda 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camjay commented Nov 15, 2021 •

edited