Annual Update for Bullseye#2
Conversation
Use the dnf task instead of yum when we detect that a Python 3 interpreter is used on a target host. Add CentOS 8 tests to the CI. Add Python 2 / Python 3 tests to the CI. Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
* update readme to reflect changes * Update README.md Co-authored-by: Lee Avital <lee.avital@datadoghq.com> * Update README.md Co-authored-by: Lee Avital <lee.avital@datadoghq.com> * Update README.md Co-authored-by: ruthnaebeck <19349244+ruthnaebeck@users.noreply.github.com> * Update README.md Co-authored-by: ruthnaebeck <19349244+ruthnaebeck@users.noreply.github.com> * Update tasks/pkg-windows-opts.yml Co-authored-by: ruthnaebeck <19349244+ruthnaebeck@users.noreply.github.com> * Update templates/system-probe.yaml.j2 Co-authored-by: ruthnaebeck <19349244+ruthnaebeck@users.noreply.github.com> * Update templates/system-probe.yaml.j2 Co-authored-by: ruthnaebeck <19349244+ruthnaebeck@users.noreply.github.com> * Update README.md Co-authored-by: ruthnaebeck <19349244+ruthnaebeck@users.noreply.github.com> * Update tasks/pkg-windows-opts.yml Co-authored-by: Albert Vaca Cintora <albert.vaca@datadoghq.com> Co-authored-by: Lee Avital <lee.avital@datadoghq.com> Co-authored-by: ruthnaebeck <19349244+ruthnaebeck@users.noreply.github.com> Co-authored-by: Albert Vaca Cintora <albert.vaca@datadoghq.com>
Redo of #326, with fixes to avoid undefined variable problems on Windows. Co-authored-by: Derek Brown <derek.brown@datadoghq.com> Co-authored-by: Lee Avital <lee.avital@datadoghq.com> Co-authored-by: ruthnaebeck <19349244+ruthnaebeck@users.noreply.github.com> Co-authored-by: Albert Vaca Cintora <albert.vaca@datadoghq.com>
Fixes the logic around starting / stopping the system-probe service, with different paths depending on the Agent version: - before 6/7.18.0: the datadog-agent-sysprobe service is independent from the datadog-agent service and is started / stopped / restarted by the role, depending on the system_probe_config.enabled variable - between 6/7.18.0 and 6/7.24.0: the datadog-agent-sysprobe service depends on datadog-agent, therefore only the datadog-agent service needs to be restarted on configuration changes. The system_probe_config.enabled variable still defines that behavior. - since 6/7.24.1: the network_config.enabled variable defines if the service should be started or not. The system_probe_config.enabled variable is still supported for backwards compatibility, but is not recommended in the documentation anymore. Makes sure the role doesn't crash when system_probe_config has a None or empty value.
- Update lint job - Fix file permissions - Add role_name and namespace to galaxy_info - Remove pre/post tasks
- Add a note about the dependency on ansible.windows since Ansible 2.10. This is because some tasks have been moved from base Ansible into separate collections. - Split Windows handler into its own file, so we don't include anything from ansible.windows on non-Windows (so we don't have to add the ansible.windows dependency on Linux as well). - Split "Gather Ansible Facts" task in two files for different Ansible versions, prefixed or unprefixed (for some reason in 2.10 it tried to run Powershell on Linux without this when using the task unprefixed as it was before).
…dog_enabled false (#338) This includes several proposed modifications: Reorder the services in the stop_services task so that when they are shut down, they don't have dependency issues due to shutdown order of operations Disable services rather than stopping them when datadog_enabled is false Ensure services are not set to disabled when datadog_enabled is true Resolve/compute datadog_tracked_checks within the task sequence as set_fact rather than only with the default vars so that its variable precedence is higher than include_vars. Also address null/none dictionary during this step. Propose calling it sanitize-checks. Co-authored-by: Kylian Serrania <kylian.serrania@datadoghq.com>
* Release version 4.8.0 Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Fixes a typo when including the main-win.yml file. This is causing errors when the role tries to notify a restart on Windows.
Dynamically includes the Windows Agent restart handler to make it work as expected.
* By default, get keys from keys.datadoghq.com, not Ubuntu keyserver * Always add the DATADOG_APT_KEY_CURRENT.public key (contains key used to sign current repodata) * Add 'signed-by' option to all sources list lines * On Debian >= 9 and Ubuntu >= 16, only add keys to /usr/share/keyrings/datadog-archive-keyring.gpg * On older systems, also add the same keyring to /etc/apt/trusted.gpg.d Co-authored-by: Kylian Serrania <kylian.serrania@datadoghq.com>
Co-authored-by: Kylian Serrania <kylian.serrania@datadoghq.com>
* This will make it compatible with Jinja2 <2.10, and Jinja2 >=3
* update windows tasks and conf * add file permissions for lint * revert granular agent restart * nit * use ansible.builtin.template w/o local tmp * add file perms * Delete .lock
The Rosetta 2 translation is no longer required for running Agent 7 in CI, since Agent 7.70+ is built "natively" for each of the 2 supported macOS architectures (`x86_64`, and now `arm64`).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLarge refactor converting many datadog variables to an Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI/Invoker
participant Role as Ansible Role (tasks/main.yml)
participant Facts as Ansible Facts
participant PKG as Package Manager (apt/dnf/zypper/win_package)
participant OS as Target OS (Linux/macOS/Windows)
participant Handler as OS Handlers (handlers/*.yml)
CI->>Role: invoke playbook / include_role
Role->>Facts: gather facts
Role->>Role: sanitize checks, parse version, set agent_datadog_* facts
Role->>PKG: include OS-specific pkg tasks (deb/rpm/dmg/msi)
PKG->>OS: download/install package
OS-->>PKG: return install result
PKG-->>Role: register install state
Role->>Handler: notify restart (OS-specific)
Handler->>OS: perform service/launchctl/win_service actions
OS-->>CI: report final state/result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
🧹 Nitpick comments (38)
.github/dependabot.yml (1)
1-6: Solid Dependabot configuration.The configuration correctly enables monthly Bundler updates with a sensible setup: v2 schema is current, the root directory is appropriate, and the monthly interval strikes a good balance between staying current and avoiding update fatigue.
Consider: Expanding to other package managers (optional).
If this repository uses Python, JavaScript, or other dependencies, you may want to add additional package ecosystems (e.g.,
pip,npm,github-actions) to the same configuration file in a follow-up PR. This keeps dependency updates centralized and maintainable.requirements.txt (1)
1-1: Consider updating to galaxy-importer 0.4.33 for the latest improvements and fixes.Version 0.4.29 is not vulnerable to the known path-traversal advisory (CVE-2023-5189) which affects only versions up to 0.4.16. However, the latest version is 0.4.33 (released August 27, 2025), so consider upgrading to benefit from intervening patches and features unless there are breaking changes that prevent it.
tasks/parse-version-macos.yml (1)
2-10: Consider more robust version parsing.The grep and awk pipeline is fragile and will fail silently if the agent's version output format changes. While
failed_when: falseprevents task failure, it may lead to empty or incorrect version detection.Consider using a more robust approach:
-- name: Get macOS Agent version - ansible.builtin.shell: - cmd: set -o pipefail && {{ datadog_agent_binary_path_macos }} version | grep 'Agent ' | awk '{print $2}' - executable: /bin/bash +- name: Get macOS Agent version + ansible.builtin.shell: + cmd: set -o pipefail && {{ datadog_agent_binary_path_macos }} version --json | jq -r '.version' + executable: /bin/bashAlternatively, if JSON output isn't available, add a validation step to check if the parsed version is non-empty.
templates/com.datadoghq.agent.plist.j2 (1)
19-25: Consider templating hard-coded paths.The binary path
/opt/datadog-agent/bin/agent/agentand log path/opt/datadog-agent/logs/launchd.logare hard-coded. If these paths might vary across deployments or versions, consider using Jinja2 variables similar to the approach used foragent_usernameandagent_groupname.Example:
<key>ProgramArguments</key> <array> - <string>/opt/datadog-agent/bin/agent/agent</string> + <string>{{ datadog_agent_binary_path_macos }}</string> <string>run</string> </array> <key>StandardOutPath</key> - <string>/opt/datadog-agent/logs/launchd.log</string> + <string>{{ datadog_agent_log_path_macos }}/launchd.log</string> <key>StandardErrorPath</key> - <string>/opt/datadog-agent/logs/launchd.log</string> + <string>{{ datadog_agent_log_path_macos }}/launchd.log</string>tasks/pkg-debian/install-installer.yml (1)
2-9: Add comment explaining theignore_errors: truerationale to match the RedHat variant.The
ignore_errors: trueis intentional and properly validated. The registered variable is checked in subsequent tasks withwhen: not ansible_check_mode and not datadog_installer_install_result.failed, and the result is used to send telemetry including installation status and error information. However, the Debian task lacks the explanatory comment present in the RedHat variant. Add a comment documenting that errors are intentionally ignored to allow telemetry collection and dependent task skipping, matching the existing documentation intasks/pkg-redhat/install-installer-dnf.yml.templates/install.json.j2 (1)
1-1: Consider adding variable validation and improving readability.The template references
install_signature.install_id,install_signature.install_time, anddatadog_apm_instrumentation_enabledwithout checking if they're defined, which could lead to template rendering errors if these variables are missing.Additionally, the single-line JSON format may reduce readability for future maintenance.
Consider applying this diff for better error handling and readability:
-{"install_id": "{{ install_signature.install_id }}", "install_type": {% if datadog_apm_instrumentation_enabled | length > 0 %}"ansible_single_step"{% else %}"ansible_manual"{% endif %}, "install_time": {{ install_signature.install_time }}} +{ + "install_id": "{{ install_signature.install_id | default('') }}", + "install_type": {% if (datadog_apm_instrumentation_enabled | default([])) | length > 0 %}"ansible_single_step"{% else %}"ansible_manual"{% endif %}, + "install_time": {{ install_signature.install_time | default(0) }} +}tasks/parse-version-windows.yml (2)
2-2: Document the rc/beta limitation in user-facing documentation.The comment notes that this task won't work with rc/beta builds. If this limitation impacts users, ensure it's documented in the role's main documentation or README.
21-21: Consider removing redundant conditional.The
when: ansible_facts.os_family == "Windows"condition is redundant sinceansible.windows.win_shellcan only execute on Windows hosts. The task will already fail gracefully on non-Windows systems.If you want to keep the explicit guard for clarity, the current implementation is acceptable. Otherwise, consider:
- when: ansible_facts.os_family == "Windows"tasks/check-removed-config.yml (2)
2-5: Consider safer type handling for the deprecation check.The condition
datadog_yum_gpgkey|length > 0will raise an error ifdatadog_yum_gpgkeyis defined but not a string or sequence (e.g., if a user sets it to a number or boolean).Consider a more defensive check:
- name: Ensure datadog_yum_gpgkey is not used ansible.builtin.fail: msg: datadog_yum_gpgkey configuration value was removed. - when: datadog_yum_gpgkey is defined and datadog_yum_gpgkey|length > 0 + when: datadog_yum_gpgkey is defined and datadog_yum_gpgkey is not none and datadog_yum_gpgkey != ''Or use:
when: datadog_yum_gpgkey is defined and datadog_yum_gpgkeyThis will catch any truthy value without type-specific operations.
7-10: Consider safer type handling for the deprecation check.Same issue as with
datadog_yum_gpgkeyabove—thelengthfilter may fail on non-sequence types.Apply the same defensive check pattern:
- name: Ensure datadog_zypper_gpgkey is not used ansible.builtin.fail: msg: datadog_zypper_gpgkey configuration value was removed. - when: datadog_zypper_gpgkey is defined and datadog_zypper_gpgkey|length > 0 + when: datadog_zypper_gpgkey is defined and datadog_zypper_gpgkey is not none and datadog_zypper_gpgkey != ''Or simply:
when: datadog_zypper_gpgkey is defined and datadog_zypper_gpgkeyCONTRIBUTING.md (2)
5-5: Modernize CI badge (Travis org is deprecated).While updating badges, consider replacing/removing the Travis badge (next line) in favor of your current CI (e.g., GitHub Actions).
66-66: Public support channel link.If the Slack workspace isn’t publicly accessible, consider pointing contributors to GitHub Discussions/Issues instead.
tasks/sanitize-checks.yml (1)
6-13: Clarify precedence and dedupe tracked checks; assert input type.Parenthesize to make intent explicit, dedupe, and validate that datadog_additional_checks is a sequence.
- agent_datadog_tracked_checks: "{{ agent_datadog_checks | list + datadog_additional_checks | default([], true) }}" + agent_datadog_tracked_checks: "{{ ((agent_datadog_checks | list) + (datadog_additional_checks | default([], true))) | unique }}"Optionally extend asserts:
ansible.builtin.assert: that: - agent_datadog_checks is mapping + - datadog_additional_checks | default([]) is sequenceci_test/downgrade_to_6.yaml (1)
5-5: Avoid absolute role path in shared examples.If not strictly CI-only, prefer a relative role reference to improve portability.
templates/zypper.repo.j2 (2)
1-7: Guard undefined datadog_zypper_repo.Safer when var is absent.
-{% if datadog_zypper_repo | length > 0 %} +{% if datadog_zypper_repo is defined and datadog_zypper_repo | length > 0 %}
16-17: Align variable naming.Mixing datadog_zypper_gpgcheck and agent_do_zypper_repo_gpgcheck is inconsistent. Consider consistent prefixes.
tasks/agent-macos.yml (1)
32-37: Consider making service check more robust.The task uses
failed_when: falseto handle cases where the service isn't running. Consider adding explicit error message handling to distinguish between "service not found" vs. otherlaunchctlfailures.tasks/integration/_linux-macos-shared.yml (1)
33-42: Consider using argv format for consistency.Lines 18-23 use
argvformat for the remove command, but line 34 uses string interpolation for the install command. For consistency and to avoid potential shell injection issues, consider usingargvformat here as well.Apply this diff:
-- name: Installing pinned version of integration, integration={{ item.key }} - ansible.builtin.command: "{{ datadog_agent_binary_path }} integration install {{ third_party }} {{ item.key }}=={{ item.value.version }}" +- name: Installing pinned version of integration, integration={{ item.key }} + ansible.builtin.command: + argv: + - "{{ datadog_agent_binary_path }}" + - integration + - install + - "{{ third_party }}" + - "{{ item.key }}=={{ item.value.version }}" become: true become_user: "{{ integration_command_user }}" vars:Note: You'll need to adjust the
third_partyvariable handling to work with argv (possibly empty string vs flag).tasks/installer-setup.yml (1)
6-9: Unusual use ofchanged_when: truefor a read-only command.The
datecommand is read-only but marked withchanged_when: true. While this ensures the task always runs and appears in change reports, it inflates the change count. Consider usingchanged_when: falsewithcheck_mode: falseif the goal is to always execute without reporting changes.Apply this diff if you want to avoid inflating change counts:
- name: Start duration measurements ansible.builtin.command: "date +%s%N" register: datadog_installer_start_time - changed_when: true + changed_when: false + check_mode: falsemanual_tests/test_7_full.yml (1)
3-3: Remove outdated Ansible version comment.The comment referencing Ansible < 1.9 is no longer relevant. Modern playbooks only need
become: yes.Apply this diff:
- - { role: ../../ansible-datadog, become: yes } # On Ansible < 1.9, use `sudo: yes` instead of `become: yes` + - { role: ../../ansible-datadog, become: yes }tasks/integration/windows.yml (1)
6-8: Fix typo in task name comment.The comment has a typo: "commmand" should be "command".
Apply this diff:
- name: Set Agent user for integration commmand + name: Set Agent user for integration commandtasks/integration/macos.yml (1)
6-8: Fix typo in task name comment.The comment has a typo: "commmand" should be "command" (same as Windows integration file).
Apply this diff:
- name: Set Agent user for integration commmand + name: Set Agent user for integration commandci_test/install_agent_7_macos.yaml (1)
10-11: Remove or update inconsistent CentOS comment in macOS test.This macOS test includes a comment about avoiding CentOS checks, which is out of place. Either remove the comment or clarify its relevance to macOS.
Apply this diff:
- # avoid checking that the agent is stopped for centos + # avoid checking that the agent is stopped for macOS (no pre-existing service)tasks/pkg-redhat/install-installer-dnf.yml (1)
9-11: Minor typo in comment.Line 11 has "depedency" which should be "dependency". While not a functional issue, it should be corrected for code quality.
Apply this diff to fix the typo:
# Since we want to send telemetry including when the installation failed, - # we need to explicitely ignore failures and skip dependent task when one - # of its depedency failed + # we need to explicitly ignore failures and skip dependent tasks when one + # of its dependencies failedtasks/set-parse-version.yml (1)
4-4: Update to modern Ansible filter syntax.The
defaultfilter's second boolean parameter can now be expressed with the named parameter syntax, which is clearer and more maintainable. Lines 4 and 12 use the positional boolean parametertrue.Consider updating to the more idiomatic named parameter approach:
- agent_datadog_agent_major_version: "{{ datadog_agent_major_version | default('', true) | string }}" + agent_datadog_agent_major_version: "{{ datadog_agent_major_version | default('', boolean=true) | string }}"This change improves readability without affecting functionality.
Also applies to: 12-12
tasks/pkg-redhat/install-pinned.yml (1)
2-5: Make disablerepo conditional to avoid harmless/noisy dnf warnings on non-AppStream hosts.On RHEL-like distros lacking an “appstream” repo id, dnf prints “No matching repo to disable”. Keep current behavior but omit the param when not applicable.
Apply this diff:
- ansible.builtin.dnf: + ansible.builtin.dnf: name: "{{ datadog_agent_flavor }}-{{ agent_datadog_agent_redhat_version }}" update_cache: true state: present allow_downgrade: "{{ datadog_agent_allow_downgrade }}" - disablerepo: "appstream" + disablerepo: "{{ (ansible_facts.os_family == 'RedHat' and (ansible_facts.distribution_major_version | int) >= 8) | ternary('appstream', omit) }}"Also applies to: 8-9
templates/security-agent.yaml.j2 (1)
1-23: LGTM; optional: simplify indentation via to_nice_yaml(indent=2).Current indent filter is fine and compatible. You could avoid Jinja indent quirks by using the filter’s built-in indent parameter instead.
Example:
runtime_security_config: {{ runtime_security_config | to_nice_yaml(indent=2) }}handlers/main-macos.yml (1)
10-22: Remove redundant check_mode and unused register; keep handler semantics clean.Tasks already gated by “when: not ansible_check_mode”; the explicit check_mode: false is redundant. Also, command_result is registered but unused.
Apply this diff:
ansible.builtin.command: "launchctl unload -wF {{ datadog_macos_system_plist_file_path }}" become: true - check_mode: false when: datadog_enabled and not ansible_check_mode and ansible_facts.os_family == "Darwin" changed_when: true @@ ansible.builtin.command: "launchctl load -wF {{ datadog_macos_system_plist_file_path }}" become: true - check_mode: false when: datadog_enabled and not ansible_check_mode and ansible_facts.os_family == "Darwin" changed_when: true @@ ansible.builtin.command: "launchctl kickstart -k system/{{ datadog_macos_service_name }}" become: true - register: command_result - check_mode: false when: datadog_enabled and not ansible_check_mode and ansible_facts.os_family == "Darwin" changed_when: trueAlso applies to: 24-30
tasks/installer-config.yml (2)
2-5: Guard against undefined variables.If
datadog_apm_instrumentation_enabledis undefined, thelengthfilter will fail. Consider adding a default value or using a defined check.Apply this diff:
- when: datadog_apm_instrumentation_enabled | length > 0 + when: datadog_apm_instrumentation_enabled | default('') | length > 0
7-19: Guard against undefined variables.The
lengthchecks on lines 16-19 will fail if the variables are undefined. Add default values to prevent errors.Apply this diff:
when: > - datadog_installer_registry | length > 0 or - datadog_installer_auth | length > 0 or - datadog_installer_version | length > 0 or - datadog_apm_inject_version | length > 0 + datadog_installer_registry | default('') | length > 0 or + datadog_installer_auth | default('') | length > 0 or + datadog_installer_version | default('') | length > 0 or + datadog_apm_inject_version | default('') | length > 0ci_test/ansible_lint.sh (1)
45-64: Consider using a more robust find pattern.Line 48 uses a for loop over
findoutput, which can be fragile with special characters in filenames. While this is likely acceptable for Ansible role files, consider usingfind -execor a while-read loop for better robustness.Alternative approach:
-for file in $(find . -type f -and \( -path '*defaults*' -or -path '*tasks*' -or -path '*templates*' -or -path '*handlers*' -or -name '.gitkeep' -or -path '*meta*' -or -name 'README.md' \)); -do +find . -type f -and \( -path '*defaults*' -or -path '*tasks*' -or -path '*templates*' -or -path '*handlers*' -or -name '.gitkeep' -or -path '*meta*' -or -name 'README.md' \) -print0 | while IFS= read -r -d '' file; do echo "Create ansible_collections/datadog/roles/agent/${file:2}" mkdir -p "$(dirname "ansible_collections/datadog/roles/agent/${file:2}")" cp -r "$file" "ansible_collections/datadog/roles/agent/${file:2}" donedefaults/main.yml (2)
261-261: Confirm ansible_user is safe to use as default for macOS user.Line 261 sets
datadog_macos_user: "{{ ansible_user }}". Theansible_uservariable is a runtime fact that may not exist in all execution contexts. Consider whether this should default to a fixed value (e.g., "dd-agent" for consistency with Linux) or include a fallback. This affects macOS agent configuration correctness.# Current (line 261): datadog_macos_user: "{{ ansible_user }}" # Consider a safer default: datadog_macos_user: "{{ ansible_user | default('dd-agent') }}"
78-117: Review RPM GPG key management strategy for expiry handling.Lines 78-117 define multiple RPM/Zypper GPG keys with expiry date comments (2022, 2024, 2028, 2033). Confirm that:
- Key rotation/removal is tracked as keys expire
- The comment on lines 101-102 about SNI and older Python is still relevant and not outdated
- Removal of expired keys (e.g., 2022 key) is handled in a migration path
tasks/agent-win.yml (1)
82-86: Verify custom checks source paths are accessible and validated.Lines 82-86 copy custom check files from
datadog_custom_checks[item]to the Windows checks directory. Confirm:
- The source paths in
datadog_custom_checksare valid and accessible from the Ansible controller- These paths are validated before reaching this task
- Error handling is in place if source files don't exist
tasks/pkg-windows.yml (1)
2-5: Verify agent_datadog_sysprobe_enabled is correctly initialized for Windows.Line 2-5 sets
agent_datadog_sysprobe_enabledfromnetwork_configbefore including Windows opts tasks. Confirm:
- This initial value is appropriate (only network_config, not other probe configs like service_monitoring_config)
- The comment "must be prior to pkg-windows-opts.yml" indicates a dependency—verify pkg-windows-opts.yml doesn't override this incorrectly
- This pattern aligns with Linux initialization (if present in agent-linux.yml)
.circleci/config.yml (3)
12-13: Verify Python path is correct for all CI environments.Lines 12-13 use
-e 'ansible_python_interpreter=/usr/bin/<<parameters.python>>'. This assumes Python is located at/usr/bin/, which may not be true for all distributions or CI images. For example:
- Alpine Linux:
/usr/local/bin/python3- Homebrew on macOS:
/opt/homebrew/bin/python3.11Consider using a more flexible approach, or ensure all CI images have Python at
/usr/bin/.
207-262: Verify APM injection test setup and dependencies.Lines 207-262 define new APM injection tests that:
- Clone the DataDog system-tests repository from GitHub (line 223)
- Build a Docker image (line 224)
- Run a test application (line 225)
- Verify traces are emitted (lines 228-239)
Confirm:
- The system-tests repository is publicly available and won't be rate-limited
- The docker build and run steps are properly integrated into CI
- The trace verification logic (jq query) is correct
- Network connectivity from CI to public repos is allowed
263-299: Verify installer test setup with fake systemctl is appropriate.Lines 263-299 (test_installer job) use a fake systemctl script to simulate systemd in a Docker container (lines 285-286). Verify:
- This approach doesn't mask real systemd-related bugs
- The datadog-installer actually needs systemctl and won't fail on the fake version
- The /etc/systemd/system/ directory creation is sufficient for the installer
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (97)
.ansible-lint(1 hunks).circleci/config.yml(3 hunks).github/CODEOWNERS(1 hunks).github/dependabot.yml(1 hunks).gitignore(1 hunks)CHANGELOG.md(4 hunks)CONTRIBUTING.md(3 hunks)Gemfile(0 hunks)README.md(16 hunks)chefignore(0 hunks)ci_test/ansible_lint.sh(1 hunks)ci_test/downgrade_to_6.yaml(1 hunks)ci_test/install_agent_6.yaml(2 hunks)ci_test/install_agent_6_macos.yaml(2 hunks)ci_test/install_agent_7.yaml(2 hunks)ci_test/install_agent_7_apm_all.yaml(1 hunks)ci_test/install_agent_7_apm_python.yaml(1 hunks)ci_test/install_agent_7_macos.yaml(2 hunks)ci_test/install_agent_7_pinned.yaml(1 hunks)ci_test/install_installer.yaml(1 hunks)ci_test/install_installer_air_gapped.yaml(1 hunks)ci_test/install_installer_over_pinned.yaml(1 hunks)ci_test/inventory/ci_macos.ini(1 hunks)defaults/main.yml(4 hunks)handlers/main-macos.yml(1 hunks)handlers/main-win.yml(1 hunks)handlers/main.yml(1 hunks)kitchen.yml(0 hunks)manual_tests/.gitignore(1 hunks)manual_tests/Vagrantfile(1 hunks)manual_tests/inventory(1 hunks)manual_tests/inventory_win(1 hunks)manual_tests/readme.md(1 hunks)manual_tests/test_5_default.yml(0 hunks)manual_tests/test_5_full.yml(0 hunks)manual_tests/test_6_default.yml(1 hunks)manual_tests/test_6_full.yml(2 hunks)manual_tests/test_7_default.yml(1 hunks)manual_tests/test_7_full.yml(1 hunks)meta/main.yml(1 hunks)requirements-ansible-lint.txt(1 hunks)requirements.txt(1 hunks)tasks/_agent-linux-macos-shared.yml(1 hunks)tasks/_apt-key-import.yml(1 hunks)tasks/_remove_rpm_keys.yml(1 hunks)tasks/agent-linux.yml(1 hunks)tasks/agent-macos.yml(1 hunks)tasks/agent-win.yml(1 hunks)tasks/agent5-linux.yml(0 hunks)tasks/check-removed-config.yml(1 hunks)tasks/installer-config.yml(1 hunks)tasks/installer-setup.yml(1 hunks)tasks/integration.yml(1 hunks)tasks/integration/_linux-macos-shared.yml(1 hunks)tasks/integration/_windows-integration-update.yml(1 hunks)tasks/integration/linux.yml(1 hunks)tasks/integration/macos.yml(1 hunks)tasks/integration/windows.yml(1 hunks)tasks/main.yml(1 hunks)tasks/os-check.yml(1 hunks)tasks/parse-version-macos.yml(1 hunks)tasks/parse-version-windows.yml(1 hunks)tasks/parse-version.yml(1 hunks)tasks/pkg-debian.yml(1 hunks)tasks/pkg-debian/install-installer.yml(1 hunks)tasks/pkg-debian/install-latest.yml(1 hunks)tasks/pkg-debian/install-pinned.yml(1 hunks)tasks/pkg-macos.yml(1 hunks)tasks/pkg-macos/macos_agent_latest.yml(1 hunks)tasks/pkg-macos/macos_agent_version.yml(1 hunks)tasks/pkg-redhat.yml(1 hunks)tasks/pkg-redhat/install-installer-dnf.yml(1 hunks)tasks/pkg-redhat/install-latest.yml(1 hunks)tasks/pkg-redhat/install-pinned.yml(1 hunks)tasks/pkg-suse.yml(1 hunks)tasks/pkg-suse/install-installer-zypper.yml(1 hunks)tasks/pkg-suse/install-latest.yml(1 hunks)tasks/pkg-suse/install-pinned.yml(1 hunks)tasks/pkg-windows-opts.yml(1 hunks)tasks/pkg-windows.yml(2 hunks)tasks/sanitize-checks.yml(1 hunks)tasks/set-parse-version.yml(1 hunks)tasks/win_agent_latest.yml(1 hunks)tasks/win_agent_version.yml(1 hunks)templates/checks.yaml.j2(1 hunks)templates/com.datadoghq.agent.plist.j2(1 hunks)templates/datadog.conf.j2(1 hunks)templates/datadog.yaml.j2(1 hunks)templates/install.json.j2(1 hunks)templates/installer-logs.j2(1 hunks)templates/installer-telemetry.j2(1 hunks)templates/security-agent.yaml.j2(1 hunks)templates/system-probe.yaml.j2(1 hunks)templates/zypper.repo.j2(2 hunks)test/integration/default/default.yml(0 hunks)test/integration/default/serverspec/localhost/default_spec.rb(0 hunks)test/integration/default/serverspec/spec_helper.rb(0 hunks)
💤 Files with no reviewable changes (9)
- tasks/agent5-linux.yml
- manual_tests/test_5_default.yml
- test/integration/default/default.yml
- Gemfile
- chefignore
- test/integration/default/serverspec/localhost/default_spec.rb
- test/integration/default/serverspec/spec_helper.rb
- manual_tests/test_5_full.yml
- kitchen.yml
🧰 Additional context used
🪛 Checkov (3.2.334)
tasks/pkg-macos/macos_agent_latest.yml
[medium] 7-22: Ensure that HTTPS url is used with uri
(CKV2_ANSIBLE_1)
🪛 LanguageTool
requirements-ansible-lint.txt
[grammar] ~1-~1: Hier könnte ein Fehler sein.
Context: ansible-lint==25.1.2
(QB_NEW_DE)
CHANGELOG.md
[uncategorized] ~9-~9: The operating system from Apple is written “macOS”.
Context: ...tation [DataDog#676] * [IMPROVEMENT] Simplify MacOS version architecture [DataDog#677] * [IMPROVEM...
(MAC_OS)
[grammar] ~21-~21: Ensure spelling is correct
Context: ...MENT] Only gather facts and services if notd done already [DataDog#659] [Thanks @POIFischba...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~53-~53: The operating system from Apple is written “macOS”.
Context: ...eb/rpms [DataDog#624] * [IMPROVEMENT] [CI] Fix MacOS CI [DataDog#630] * [BUGFIX] Fix Ansible check ...
(MAC_OS)
[grammar] ~86-~86: Ensure spelling is correct
Context: ...ll.json file related to apm single step instrumation [DataDog#572] * [FEATURE] add support for data...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~150-~150: Use a hyphen to join words.
Context: ...ng includepkgs in repofiles on Red Hat compatible platforms. See [DataDog#443]. Thanks...
(QB_NEW_EN_HYPHEN)
[grammar] ~155-~155: Use a hyphen to join words.
Context: ...ing role multiple times in a row on SUSE compatible platforms. See [DataDog#453]. * [DOC...
(QB_NEW_EN_HYPHEN)
README.md
[style] ~112-~112: ‘Whether or not’ might be wordy. Consider a shorter alternative.
Context: ...og_apply_windows_614_fix` | Whether or not to download and apply file referenced b...
(EN_WORDINESS_PREMIUM_WHETHER_OR_NOT)
[uncategorized] ~113-~113: The operating system from Apple is written “macOS”.
Context: ...ren't running Datadog Agent 6.14.*.| | datadog_macos_user | The name...
(MAC_OS)
[uncategorized] ~114-~114: The operating system from Apple is written “macOS”.
Context: ...ults to ansible_user (macOS only).| | datadog_macos_download_url | Override...
(MAC_OS)
[grammar] ~122-~122: Use a hyphen to join words.
Context: ... the datadog_checks section. The first level key is the name of the check, and ...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~330-~330: The operating system from Apple is written “macOS”.
Context: ... string. #### macOS When the variable datadog_macos_download_url is not set, the official ...
(MAC_OS)
[grammar] ~358-~358: Use a hyphen to join words.
Context: ...e`: Removes an integration. ##### Third party integrations [Datadog community][...
(QB_NEW_EN_HYPHEN)
[style] ~580-~580: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...rmation applies to versions of the role prior to 4.9.0. Since 4.9.0, the apt_key modul...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
32-32: Bare URL used
(MD034, no-bare-urls)
README.md
112-112: Bare URL used
(MD034, no-bare-urls)
315-315: Bare URL used
(MD034, no-bare-urls)
323-323: Bare URL used
(MD034, no-bare-urls)
324-324: Bare URL used
(MD034, no-bare-urls)
334-334: Bare URL used
(MD034, no-bare-urls)
335-335: Bare URL used
(MD034, no-bare-urls)
336-336: Bare URL used
(MD034, no-bare-urls)
337-337: Bare URL used
(MD034, no-bare-urls)
347-347: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
628-628: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
646-646: Link and image reference definitions should be needed
Unused link or image reference definition: "2"
(MD053, link-image-reference-definitions)
🪛 Shellcheck (0.11.0)
ci_test/ansible_lint.sh
[warning] 12-12: Quote this to prevent word splitting.
(SC2046)
[warning] 48-48: For loops over find output are fragile. Use find -exec or a while read loop.
(SC2044)
| - test_install: | ||
| matrix: | ||
| parameters: | ||
| ansible_version: ["2_10", "3_4", "4_10", "9_4"] | ||
| agent_version: ["6", "7"] | ||
| jinja2_native: ["true", "false"] | ||
| os: ["rocky8"] | ||
| python: ["python3"] | ||
|
|
||
| - test_install: | ||
| matrix: | ||
| parameters: | ||
| ansible_version: ["12_0"] | ||
| agent_version: ["6", "7"] | ||
| jinja2_native: ["true", "false"] | ||
| os: ["rocky9", "suse", "amazonlinux2023"] | ||
| python: ["python3.11"] | ||
|
|
||
| # Newer suse images only have Python 3 installed | ||
| - test_install: | ||
| matrix: | ||
| parameters: | ||
| ansible_version: ["2_8", "2_9", "2_10"] | ||
| ansible_version: ["2_10", "3_4", "4_10", "9_4"] | ||
| agent_version: ["6", "7"] | ||
| os: ["suse"] | ||
| python: ["python3"] | ||
|
|
||
| - test_install: | ||
| matrix: | ||
| parameters: | ||
| ansible_version: ["2_10", "3_4", "4_10"] | ||
| agent_version: ["6", "7"] | ||
| os: ["amazonlinux2023"] | ||
| python: ["python3"] | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Review test matrix coverage and Python version consistency.
Lines 469-503 expand the test matrices to include multiple Ansible versions, agent versions, distributions, and Python versions. Verify that:
- Python versions align with distribution capabilities (python3 for older Debian, python3.11 for newer)
- Ansible versions support the Python versions they're paired with (Ansible 5.x requires Python >= 3.8)
- The jinja2_native parameter is tested across a representative sample of combinations
- macOS tests are included (line 504+)
🏁 Script executed:
#!/bin/bash
# Count matrix combinations to ensure coverage is reasonable
echo "Test matrix combinations:"
rg -o 'parameters:.*python: .*' .circleci/config.yml | wc -lLength of output: 27
I need to examine the actual file structure more carefully, as the regex pattern didn't work with the YAML format.
---
🏁 Script executed:
```shell
#!/bin/bash
# Extract the relevant section of the config file with better context
echo "=== Lines 465-510 of .circleci/config.yml ==="
sed -n '465,510p' .circleci/config.yml
Length of output: 1449
🏁 Script executed:
#!/bin/bash
# Search for macOS test configurations
echo "=== Checking for macOS test references ==="
rg -n "macos|darwin" .circleci/config.yml -iLength of output: 349
🏁 Script executed:
#!/bin/bash
# Search for all test_install matrix definitions to understand structure
echo "=== All test_install matrix blocks ==="
rg -n "test_install:" .circleci/config.yml -A 10Length of output: 1589
🏁 Script executed:
#!/bin/bash
# Check what comes after line 503
echo "=== Lines 500-520 of config ==="
sed -n '500,520p' .circleci/config.ymlLength of output: 613
🏁 Script executed:
#!/bin/bash
# Look for ansible version definitions and python compatibility info
echo "=== Checking ansible_version parameter usage ==="
rg -n "ansible_version" .circleci/config.yml -C 3Length of output: 9461
🏁 Script executed:
#!/bin/bash
# Search for jinja2_native usage
echo "=== jinja2_native parameter coverage ==="
rg -n "jinja2_native" .circleci/config.yml -C 2Length of output: 4286
🏁 Script executed:
#!/bin/bash
# Check python version specifications
echo "=== Python version parameters ==="
rg -n "python.*:" .circleci/config.yml | head -30Length of output: 1248
🏁 Script executed:
#!/bin/bash
# Get file length and overall structure
echo "=== File statistics ==="
wc -l .circleci/config.yml
echo ""
echo "=== Test matrix sections ==="
rg -n "- test_install:" .circleci/config.ymlLength of output: 483
I need to verify the Ansible version compatibility claims:
I need to check if Rocky Linux and SUSE default Python versions could support Ansible 9.4:<!-- [scratchpad_update]
[task verify_review_comment_1/1 COMPLETE]
[observations]
Key findings from web searches:
- Rocky Linux 8: No default version of Python on Rocky Linux 8, but Python 3.6.8 if installed seems to take precedence. Available versions are 3.6, 3.8, 3.9, 3.12 (from source 21-15, 28-1, 28-4)
- Ansible 9 removed Python 3.9 support; Python 3.10 or newer is required
- SUSE: SUSE Linux Enterprise Server 15 uses Python 3.6 by default, Python 3.9 added in SP3 but no longer supported as of SP4 (sources 32-1, 32-2, 32-3, 35-1)
Issues identified:
-
Ansible 9.4 (9_4 notation) is tested with generic "python3" on rocky8 (line 472) and suse (line 491)
- Rocky8 default is 3.6.8, which is below Ansible 9's 3.10+ requirement
- SUSE default is 3.6, also insufficient for Ansible 9.4
- This is a CRITICAL issue
-
jinja2_native coverage incomplete:
- Only tested in 2 of 4 matrices (rocky8 and ansible 12.0 combination)
- NOT tested for suse standalone or amazonlinux2023
- Partial coverage issue
-
macOS tests present and correct format with all Ansible versions
- But also uses generic "python3" (though brew installs 3.11)
[analysis]
Review comment concerns are partially valid:
- But also uses generic "python3" (though brew installs 3.11)
- Issue 1 (Python alignment): INCORRECT for Ansible 9.4 - python3 on rocky8/suse insufficient
- Issue 2 (Ansible compatibility): CRITICAL ISSUE FOUND - Ansible 9.4 requires Python 3.10+
- Issue 3 (jinja2_native coverage): VALID but partial - not all combinations tested
- Issue 4 (macOS tests): VALID - tests present
The review comment is asking for verification, but actual code issues exist that need to be flagged.
[actions]
- Generate rewritten comment highlighting the Ansible 9.4 Python version issue
- Flag jinja2_native coverage gaps
- Confirm macOS tests are present
[scratchpad_end] -->
Ansible 9.4 Python version incompatibility detected. Consider explicit Python 3.10+ for affected tests.
The test matrices reveal a compatibility issue: Ansible 9 requires Python 3.10 or newer on the controller, but lines 472 and 491 pair ansible_version "9_4" with generic python: ["python3"] on rocky8 and suse. Rocky Linux 8's python3 link defaults to Python 3.6.8, and SUSE Linux Enterprise Server 15 uses Python 3.6 by default—both insufficient.
Additionally, jinja2_native parameter testing is incomplete: it's tested with Ansible 2_10–9_4 on rocky8 and with Ansible 12_0 across rocky9/suse/amazonlinux2023, but not for suse or amazonlinux2023 in standalone matrices (lines 488–502).
Confirm: Upgrade python specification for Ansible 9.4 tests, or skip those combinations on distributions with Python < 3.10. macOS tests (line 504+) are included as expected.
| * [IMPROVEMENT] Remove pycache directory during upgrades [#673] | ||
| * [IMPROVEMENT] Prepare deployment of Agent v7.70+ for macOS (arm64/x86_64) [664] | ||
| * [DOCUMENTATION] Adjust misleading macOS Agent version documentation [#676] | ||
| * [IMPROVEMENT] Simplify MacOS version architecture [#677] |
There was a problem hiding this comment.
Fix capitalization: use "macOS" instead of "MacOS".
The operating system from Apple is written as "macOS" (lowercase 'm').
Apply this diff:
-* [IMPROVEMENT] Simplify MacOS version architecture [#677]
+* [IMPROVEMENT] Simplify macOS version architecture [#677]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * [IMPROVEMENT] Simplify MacOS version architecture [#677] | |
| * [IMPROVEMENT] Simplify macOS version architecture [#677] |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: The operating system from Apple is written “macOS”.
Context: ...tation [DataDog#676] * [IMPROVEMENT] Simplify MacOS version architecture [DataDog#677] * [IMPROVEM...
(MAC_OS)
🤖 Prompt for AI Agents
In CHANGELOG.md around line 9, the entry uses the incorrect capitalization
"MacOS"; change it to the correct Apple branding "macOS" so the line reads "*
[IMPROVEMENT] Simplify macOS version architecture [#677]". Ensure only the
capitalization of "macOS" is updated and nothing else in the line is modified.
| # 5.2.1 / 2025-05-22 | ||
| * [IMPROVEMENT] Provide bootstrap environment variables to installer only if not empty strings [#655] | ||
| * [IMPROVEMENT] Remove community.general.zypper reference [#658] | ||
| * [IMPROVEMENT] Only gather facts and services if notd done already [#659] [Thanks @POIFischbacher] |
There was a problem hiding this comment.
Fix typo: "notd" should be "not".
Apply this diff:
-* [IMPROVEMENT] Only gather facts and services if notd done already [#659] [Thanks @POIFischbacher]
+* [IMPROVEMENT] Only gather facts and services if not done already [#659] [Thanks @POIFischbacher]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * [IMPROVEMENT] Only gather facts and services if notd done already [#659] [Thanks @POIFischbacher] | |
| * [IMPROVEMENT] Only gather facts and services if not done already [#659] [Thanks @POIFischbacher] |
🧰 Tools
🪛 LanguageTool
[grammar] ~21-~21: Ensure spelling is correct
Context: ...MENT] Only gather facts and services if notd done already [DataDog#659] [Thanks @POIFischba...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
In CHANGELOG.md around line 21, there's a typo: the word "notd" should be
corrected to "not"; update the line so it reads "Only gather facts and services
if not done already" preserving the rest of the entry and formatting.
| * [IMPROVEMENT] Provide bootstrap environment variables to installer only if not empty strings [#655] | ||
| * [IMPROVEMENT] Remove community.general.zypper reference [#658] | ||
| * [IMPROVEMENT] Only gather facts and services if notd done already [#659] [Thanks @POIFischbacher] | ||
| * [IMROVEMENT] [CI] Remove role prefix lint warnings [#660] |
There was a problem hiding this comment.
Fix typo: "IMROVEMENT" should be "IMPROVEMENT".
Apply this diff:
-* [IMROVEMENT] [CI] Remove role prefix lint warnings [#660]
+* [IMPROVEMENT] [CI] Remove role prefix lint warnings [#660]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * [IMROVEMENT] [CI] Remove role prefix lint warnings [#660] | |
| * [IMPROVEMENT] [CI] Remove role prefix lint warnings [#660] |
🤖 Prompt for AI Agents
In CHANGELOG.md around line 22, the word "IMROVEMENT" is misspelled; change
"IMROVEMENT" to the correct "IMPROVEMENT" so the entry reads "* [IMPROVEMENT]
[CI] Remove role prefix lint warnings [#660]". Ensure only the typo is fixed and
keep the rest of the line intact.
| ansible.builtin.set_fact: | ||
| agent_dd_download_url: "{% if agent_datadog_agent_major_version | ||
| | int == 7 %}{{ datadog_windows_agent7_latest_url }} {% else %}{{ datadog_windows_agent6_latest_url }}{% endif %}" | ||
| when: datadog_windows_download_url | default('', true) | length == 0 |
There was a problem hiding this comment.
Trailing space in Jinja URL (version==7) can break downloads.
There’s a literal space after datadog_windows_agent7_latest_url. Remove it or switch to ternary.
Apply one of the following:
- agent_dd_download_url: "{% if agent_datadog_agent_major_version
- | int == 7 %}{{ datadog_windows_agent7_latest_url }} {% else %}{{ datadog_windows_agent6_latest_url }}{% endif %}"
+ agent_dd_download_url: "{% if (agent_datadog_agent_major_version | int) == 7 %}{{ datadog_windows_agent7_latest_url }}{% else %}{{ datadog_windows_agent6_latest_url }}{% endif %}"Or:
- agent_dd_download_url: "{% if agent_datadog_agent_major_version
- | int == 7 %}{{ datadog_windows_agent7_latest_url }} {% else %}{{ datadog_windows_agent6_latest_url }}{% endif %}"
+ agent_dd_download_url: "{{ ((agent_datadog_agent_major_version | int) == 7)
+ | ternary(datadog_windows_agent7_latest_url, datadog_windows_agent6_latest_url) }}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ansible.builtin.set_fact: | |
| agent_dd_download_url: "{% if agent_datadog_agent_major_version | |
| | int == 7 %}{{ datadog_windows_agent7_latest_url }} {% else %}{{ datadog_windows_agent6_latest_url }}{% endif %}" | |
| when: datadog_windows_download_url | default('', true) | length == 0 | |
| ansible.builtin.set_fact: | |
| agent_dd_download_url: "{% if (agent_datadog_agent_major_version | int) == 7 %}{{ datadog_windows_agent7_latest_url }}{% else %}{{ datadog_windows_agent6_latest_url }}{% endif %}" | |
| when: datadog_windows_download_url | default('', true) | length == 0 |
| ansible.builtin.set_fact: | |
| agent_dd_download_url: "{% if agent_datadog_agent_major_version | |
| | int == 7 %}{{ datadog_windows_agent7_latest_url }} {% else %}{{ datadog_windows_agent6_latest_url }}{% endif %}" | |
| when: datadog_windows_download_url | default('', true) | length == 0 | |
| ansible.builtin.set_fact: | |
| agent_dd_download_url: "{{ ((agent_datadog_agent_major_version | int) == 7) | |
| | ternary(datadog_windows_agent7_latest_url, datadog_windows_agent6_latest_url) }}" | |
| when: datadog_windows_download_url | default('', true) | length == 0 |
🤖 Prompt for AI Agents
In tasks/win_agent_latest.yml around lines 8 to 11, the Jinja expression sets
agent_dd_download_url with a literal trailing space after {{
datadog_windows_agent7_latest_url }} which can break downloads; remove the
trailing space (i.e., change the template so the version==7 branch has no
trailing space) or replace the if/else block with a ternary expression such as
{{ (agent_datadog_agent_major_version | int == 7) |
ternary(datadog_windows_agent7_latest_url, datadog_windows_agent6_latest_url) }}
to ensure no accidental whitespace is included.
| {% if agent_datadog_config is not defined or agent_datadog_config["api_key"] is not defined -%} | ||
| api_key: {{ datadog_api_key }} | ||
| {% endif %} |
There was a problem hiding this comment.
Guard api_key emission; current block fails when datadog_api_key is undefined.
If api_key isn’t set in agent_datadog_config and datadog_api_key is not defined, templating errors out. Add a defined check.
Apply this diff:
-{% if agent_datadog_config is not defined or agent_datadog_config["api_key"] is not defined -%}
-api_key: {{ datadog_api_key }}
+{% if (agent_datadog_config is not defined or agent_datadog_config["api_key"] is not defined)
+ and (datadog_api_key is defined) -%}
+api_key: "{{ datadog_api_key }}"
{% endif %}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {% if agent_datadog_config is not defined or agent_datadog_config["api_key"] is not defined -%} | |
| api_key: {{ datadog_api_key }} | |
| {% endif %} | |
| {% if (agent_datadog_config is not defined or agent_datadog_config["api_key"] is not defined) | |
| and (datadog_api_key is defined) -%} | |
| api_key: "{{ datadog_api_key }}" | |
| {% endif %} |
🤖 Prompt for AI Agents
In templates/datadog.yaml.j2 around lines 13 to 15, the current conditional
emits api_key when agent_datadog_config is missing or lacks api_key but assumes
datadog_api_key exists, causing template errors if datadog_api_key is undefined;
update the if condition to also check that datadog_api_key is defined (e.g., add
"and datadog_api_key is defined") so the api_key line is only rendered when
datadog_api_key is present.
| { | ||
| "api_version": "v2", | ||
| "request_type": "logs", | ||
| "tracer_time": "{{ stop_time }}", | ||
| "runtime_id": "{{ trace_id }}", | ||
| "seq_id": 2, | ||
| "origin": "ansible", | ||
| "host": { | ||
| "hostname": "{{ hostname }}", | ||
| "os": "{{ system }}", | ||
| "distribution": "{{ os_family }}", | ||
| "architecture": "{{ arch }}", | ||
| "kernel_version": "{{ kernel }}", | ||
| "kernel_release": "{{ kernel_version }}" | ||
| }, | ||
| "application": { | ||
| "service_name": "datadog-ansible", | ||
| "service_version": "{{ role_version }}", | ||
| "language_name": "UNKNOWN", | ||
| "language_version": "n/a", | ||
| "tracer_version": "n/a" | ||
| }, | ||
| "payload": { | ||
| "logs": [{ | ||
| "message": "{{ stdout }}", | ||
| "level": "DEBUG", | ||
| "trace_id": {{ datadog_installer_trace_id }}, | ||
| "span_id": {{ datadog_installer_trace_id }} | ||
| },{ | ||
| "message": "{{ stderr }}", | ||
| "level": "ERROR", | ||
| "trace_id": {{ datadog_installer_trace_id }}, | ||
| "span_id": {{ datadog_installer_trace_id }} | ||
| }] | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Escape strings and cast numbers to produce valid JSON (and reduce PII risk).
Use tojson for strings and int casts for IDs/timestamps; optionally redact secrets in stderr.
- "tracer_time": "{{ stop_time }}",
- "runtime_id": "{{ trace_id }}",
+ "tracer_time": {{ stop_time | int }},
+ "runtime_id": {{ (trace_id | string) | tojson }},
@@
- "hostname": "{{ hostname }}",
- "os": "{{ system }}",
- "distribution": "{{ os_family }}",
- "architecture": "{{ arch }}",
- "kernel_version": "{{ kernel }}",
- "kernel_release": "{{ kernel_version }}"
+ "hostname": {{ hostname | tojson }},
+ "os": {{ system | tojson }},
+ "distribution": {{ os_family | tojson }},
+ "architecture": {{ arch | tojson }},
+ "kernel_version": {{ kernel | tojson }},
+ "kernel_release": {{ kernel_version | tojson }}
@@
- "message": "{{ stdout }}",
+ "message": {{ stdout | tojson }},
"level": "DEBUG",
- "trace_id": {{ datadog_installer_trace_id }},
- "span_id": {{ datadog_installer_trace_id }}
+ "trace_id": {{ datadog_installer_trace_id | int }},
+ "span_id": {{ datadog_installer_trace_id | int }}
@@
- "message": "{{ stderr }}",
+ "message": {{ (stderr
+ | regex_replace('(?i)apikey\\s*[:=]\\s*\\S+', 'apikey=***REDACTED***')
+ | tojson) }},
"level": "ERROR",
- "trace_id": {{ datadog_installer_trace_id }},
- "span_id": {{ datadog_installer_trace_id }}
+ "trace_id": {{ datadog_installer_trace_id | int }},
+ "span_id": {{ datadog_installer_trace_id | int }}Please validate payloads with jq:
🏁 Script executed:
#!/bin/bash
jq empty templates/installer-logs.j2 >/dev/null || echo "Template contains non-JSON constructs; test via render-in-play."Length of output: 122
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect the actual template file to confirm current state and line numbers
cat -n templates/installer-logs.j2
Length of output: 1339
---
🌐 Web query:
```web
Ansible Jinja2 filters tojson int regex_replace
💡 Result:
Here are the Ansible/Jinja2 filter usages and short examples.
-
regex_replace — replace using a regular expression. Use either short name or FQCN ansible.builtin.regex_replace. Example:
- "{{ 'localhost:80' | regex_replace('^(?P.+):(?P\d+)$', '\g, \g') }}" — yields "localhost, 80". [1]
-
tojson / to_json — serialize a variable to a JSON string (marks safe for HTML in Jinja2). Example:
- "{{ myvar | tojson }}" or "{{ myvar | to_json }}". [2]
-
int — convert a value to an integer. Example:
- "{{ '42' | int }}" yields integer 42. [3]
-
Chaining example (common pattern): extract digits with regex_replace then convert to int:
- "{{ 'size=42MB' | regex_replace('.?(\d+).','\1') | int }}" — yields 42. [1][3]
Notes:
- Escape backslashes in YAML/Jinja strings (\1, \g) as shown above. [1]
- Use FQCNs (ansible.builtin.) if you want unambiguous references in docs/playbooks. [1][2][3]
Sources: Ansible docs for regex_replace, tojson/to_json and int. [1][2][3]
Fix invalid JSON by escaping strings and casting numeric fields.
The template currently produces invalid JSON due to unescaped strings and untyped numeric fields. Apply the following:
- Cast
tracer_timeanddatadog_installer_trace_idvalues to integers using| int - Escape all string interpolations (hostname, system, os_family, etc.) and log messages using
| tojson - Optionally redact API keys from stderr using
| regex_replace('(?i)apikey\\s*[:=]\\s*\\S+', 'apikey=***REDACTED***')
- "tracer_time": "{{ stop_time }}",
- "runtime_id": "{{ trace_id }}",
+ "tracer_time": {{ stop_time | int }},
+ "runtime_id": {{ (trace_id | string) | tojson }},
@@
- "hostname": "{{ hostname }}",
- "os": "{{ system }}",
- "distribution": "{{ os_family }}",
- "architecture": "{{ arch }}",
- "kernel_version": "{{ kernel }}",
- "kernel_release": "{{ kernel_version }}"
+ "hostname": {{ hostname | tojson }},
+ "os": {{ system | tojson }},
+ "distribution": {{ os_family | tojson }},
+ "architecture": {{ arch | tojson }},
+ "kernel_version": {{ kernel | tojson }},
+ "kernel_release": {{ kernel_version | tojson }}
@@
- "message": "{{ stdout }}",
+ "message": {{ stdout | tojson }},
"level": "DEBUG",
- "trace_id": {{ datadog_installer_trace_id }},
- "span_id": {{ datadog_installer_trace_id }}
+ "trace_id": {{ datadog_installer_trace_id | int }},
+ "span_id": {{ datadog_installer_trace_id | int }}
@@
- "message": "{{ stderr }}",
+ "message": {{ (stderr
+ | regex_replace('(?i)apikey\\s*[:=]\\s*\\S+', 'apikey=***REDACTED***')
+ | tojson) }},
"level": "ERROR",
- "trace_id": {{ datadog_installer_trace_id }},
- "span_id": {{ datadog_installer_trace_id }}
+ "trace_id": {{ datadog_installer_trace_id | int }},
+ "span_id": {{ datadog_installer_trace_id | int }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "api_version": "v2", | |
| "request_type": "logs", | |
| "tracer_time": "{{ stop_time }}", | |
| "runtime_id": "{{ trace_id }}", | |
| "seq_id": 2, | |
| "origin": "ansible", | |
| "host": { | |
| "hostname": "{{ hostname }}", | |
| "os": "{{ system }}", | |
| "distribution": "{{ os_family }}", | |
| "architecture": "{{ arch }}", | |
| "kernel_version": "{{ kernel }}", | |
| "kernel_release": "{{ kernel_version }}" | |
| }, | |
| "application": { | |
| "service_name": "datadog-ansible", | |
| "service_version": "{{ role_version }}", | |
| "language_name": "UNKNOWN", | |
| "language_version": "n/a", | |
| "tracer_version": "n/a" | |
| }, | |
| "payload": { | |
| "logs": [{ | |
| "message": "{{ stdout }}", | |
| "level": "DEBUG", | |
| "trace_id": {{ datadog_installer_trace_id }}, | |
| "span_id": {{ datadog_installer_trace_id }} | |
| },{ | |
| "message": "{{ stderr }}", | |
| "level": "ERROR", | |
| "trace_id": {{ datadog_installer_trace_id }}, | |
| "span_id": {{ datadog_installer_trace_id }} | |
| }] | |
| } | |
| } | |
| { | |
| "api_version": "v2", | |
| "request_type": "logs", | |
| "tracer_time": {{ stop_time | int }}, | |
| "runtime_id": {{ (trace_id | string) | tojson }}, | |
| "seq_id": 2, | |
| "origin": "ansible", | |
| "host": { | |
| "hostname": {{ hostname | tojson }}, | |
| "os": {{ system | tojson }}, | |
| "distribution": {{ os_family | tojson }}, | |
| "architecture": {{ arch | tojson }}, | |
| "kernel_version": {{ kernel | tojson }}, | |
| "kernel_release": {{ kernel_version | tojson }} | |
| }, | |
| "application": { | |
| "service_name": "datadog-ansible", | |
| "service_version": "{{ role_version }}", | |
| "language_name": "UNKNOWN", | |
| "language_version": "n/a", | |
| "tracer_version": "n/a" | |
| }, | |
| "payload": { | |
| "logs": [{ | |
| "message": {{ stdout | tojson }}, | |
| "level": "DEBUG", | |
| "trace_id": {{ datadog_installer_trace_id | int }}, | |
| "span_id": {{ datadog_installer_trace_id | int }} | |
| },{ | |
| "message": {{ (stderr | |
| | regex_replace('(?i)apikey\\s*[:=]\\s*\\S+', 'apikey=***REDACTED***') | |
| | tojson) }}, | |
| "level": "ERROR", | |
| "trace_id": {{ datadog_installer_trace_id | int }}, | |
| "span_id": {{ datadog_installer_trace_id | int }} | |
| }] | |
| } | |
| } |
🤖 Prompt for AI Agents
In templates/installer-logs.j2 around lines 1 to 36, the template emits invalid
JSON because string interpolations are not JSON-escaped and numeric fields are
left untyped; fix it by casting tracer_time and all trace/span id interpolations
to integers with |int, wrap every string interpolation (hostname, system,
os_family, arch, kernel, kernel_version, role_version, stdout, stderr, etc.)
with |tojson to ensure proper escaping, and for stderr optionally pipe through
|regex_replace('(?i)apikey\\s*[:=]\\s*\\S+', 'apikey=***REDACTED***') before
tojson to redact API keys; ensure trace_id/span_id fields remain unquoted
numeric values after casting.
| { | ||
| "api_version": "v2", | ||
| "request_type": "traces", | ||
| "tracer_time": {{ stop_time }}, | ||
| "runtime_id": "{{ trace_id }}", | ||
| "seq_id": 1, | ||
| "origin": "ansible", | ||
| "host": { | ||
| "hostname": "{{ hostname }}", | ||
| "os": "{{ system }}", | ||
| "distribution": "{{ os_family }}", | ||
| "architecture": "{{ arch }}", | ||
| "kernel_version": "{{ kernel }}", | ||
| "kernel_release": "{{ kernel_version }}" | ||
| }, | ||
| "application": { | ||
| "service_name": "datadog-ansible", | ||
| "service_version": "{{ role_version }}", | ||
| "language_name": "UNKNOWN", | ||
| "language_version": "n/a", | ||
| "tracer_version": "n/a" | ||
| }, | ||
| "payload": { | ||
| "traces": [[{ | ||
| "service": "datadog-ansible", | ||
| "name": "install_installer", | ||
| "resource": "install_installer", | ||
| "trace_id": {{ trace_id }}, | ||
| "span_id": {{ trace_id }}, | ||
| "parent_id": 0, | ||
| "start": {{ start_time }}, | ||
| "duration": {{ stop_time - start_time }}, | ||
| "error": {{ rc }}, | ||
| "meta": { | ||
| "language": "yaml", | ||
| "exit_code": {{ rc }}, | ||
| "error": { | ||
| "message": "{{ stderr }}" | ||
| }, | ||
| "version": "{{ role_version }}" | ||
| }, | ||
| "metrics": { | ||
| "_trace_root": 1, | ||
| "_top_level": 1, | ||
| "_dd.top_level": 1, | ||
| "_sampling_priority_v1": 2 | ||
| } | ||
| }]] | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Harden JSON: escape strings and cast numerics; redact stderr.
Apply tojson for strings and int casts for numbers to avoid invalid JSON; redact obvious secrets in stderr.
- "tracer_time": {{ stop_time }},
- "runtime_id": "{{ trace_id }}",
+ "tracer_time": {{ stop_time | int }},
+ "runtime_id": {{ (trace_id | string) | tojson }},
@@
- "hostname": "{{ hostname }}",
- "os": "{{ system }}",
- "distribution": "{{ os_family }}",
- "architecture": "{{ arch }}",
- "kernel_version": "{{ kernel }}",
- "kernel_release": "{{ kernel_version }}"
+ "hostname": {{ hostname | tojson }},
+ "os": {{ system | tojson }},
+ "distribution": {{ os_family | tojson }},
+ "architecture": {{ arch | tojson }},
+ "kernel_version": {{ kernel | tojson }},
+ "kernel_release": {{ kernel_version | tojson }}
@@
- "trace_id": {{ trace_id }},
- "span_id": {{ trace_id }},
+ "trace_id": {{ trace_id | int }},
+ "span_id": {{ trace_id | int }},
@@
- "start": {{ start_time }},
- "duration": {{ stop_time - start_time }},
- "error": {{ rc }},
+ "start": {{ start_time | int }},
+ "duration": {{ (stop_time - start_time) | int }},
+ "error": {{ rc | int }},
@@
- "message": "{{ stderr }}"
+ "message": {{ (stderr
+ | regex_replace('(?i)apikey\\s*[:=]\\s*\\S+', 'apikey=***REDACTED***')
+ | tojson) }}Render a sample payload and validate with jq:
🏁 Script executed:
#!/bin/bash
ansible -m template -a 'src=templates/installer-telemetry.j2 dest=/tmp/payload.json' localhost -e '{"trace_id":123,"start_time":1,"stop_time":2,"rc":0,"hostname":"h","system":"s","os_family":"f","arch":"a","kernel":"k","kernel_version":"kv","role_version":"v","stderr":""}' -c local
jq empty /tmp/payload.json && echo "OK"Length of output: 121
I'll verify the template file and confirm the Jinja2 filters suggested in the review.
---
🏁 Script executed:
```shell
#!/bin/bash
# Read the template file to check current content
if [ -f "templates/installer-telemetry.j2" ]; then
cat "templates/installer-telemetry.j2"
else
echo "File not found"
fi
Length of output: 1509
🌐 Web query:
Jinja2 tojson filter regex_replace string int filters
💡 Result:
Short summary + examples.
Key points
- You can chain Jinja2 filters left→right; the output of one is input to the next. [1]
- tojson serializes a value to JSON and marks it safe for HTML/script contexts (escapes <, >, &, '). It accepts an optional indent. [2]
- regex_replace is available (or can be added as a custom filter) to run a Python regex replacement; use \1 for backreferences in the replacement string. [3]
- string and int are built‑in cast filters to convert types. [1][3]
Examples
-
Remove non-digits, cast to int:
{{ val|regex_replace('[^0-9]', '')|int }} -
Cast to string then JSON-encode (safe for embedding in <script>):
<script>const data = {{ value|string|tojson }};</script> -
Replace prefix with nothing, then JSON:
{{ mystr|regex_replace('^prefix-', '')|tojson }} -
Use capture groups in replacement:
{{ text|regex_replace('([a-z]+)-([0-9]+)', '\2-\1') }}
Notes
- If your environment lacks regex_replace you can register a custom filter or use replace for simple literal replaces. [3]
- tojson is intended for HTML/script output; for non-HTML contexts use a normal JSON dump in your backend if needed. [2]
Sources
- Jinja2 filters reference (built-ins, casting, chaining). [1]
- tojson description/behavior (escaping, indent). [2]
- regex/regex_replace usage and examples (including backreferences / custom filter guidance). [3]
Would you like a concrete example using your actual template data?
Apply suggested JSON hardening filters to all template variables.
The template file matches the review snippet and all identified issues are valid:
- String variables (
hostname,system, etc.) andstderrare unescaped and unquoted, risking JSON breakage if they contain special characters. - Numeric variables (
trace_id,start_time,stop_time,rc) lackintcasting, risking type coercion. - The
stderrfield should be redacted to prevent API key leakage.
All suggested Jinja2 filters (tojson, int, string, regex_replace) are valid and available. Apply the provided diffs to harden JSON output.
| gpgkey={{ datadog_zypper_gpgkey_current }} | ||
| {{ datadog_zypper_gpgkey_20280418 }} | ||
| {{ datadog_zypper_gpgkey_20200908 }} | ||
| {{ datadog_zypper_gpgkey_e09422b3 }} | ||
| {% endif %} |
There was a problem hiding this comment.
gpgkey must be a single logical line; join keys for SUSE ≥ 15.
Multiple physical lines under one key likely won’t parse. Join with spaces.
-{% else %}
-gpgkey={{ datadog_zypper_gpgkey_current }}
- {{ datadog_zypper_gpgkey_20280418 }}
- {{ datadog_zypper_gpgkey_20200908 }}
- {{ datadog_zypper_gpgkey_e09422b3 }}
+{% else %}
+gpgkey={{ [datadog_zypper_gpgkey_current,
+ datadog_zypper_gpgkey_20280418,
+ datadog_zypper_gpgkey_20200908,
+ datadog_zypper_gpgkey_e09422b3] | join(' ') }}
{% endif %}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gpgkey={{ datadog_zypper_gpgkey_current }} | |
| {{ datadog_zypper_gpgkey_20280418 }} | |
| {{ datadog_zypper_gpgkey_20200908 }} | |
| {{ datadog_zypper_gpgkey_e09422b3 }} | |
| {% endif %} | |
| {% else %} | |
| gpgkey={{ [datadog_zypper_gpgkey_current, | |
| datadog_zypper_gpgkey_20280418, | |
| datadog_zypper_gpgkey_20200908, | |
| datadog_zypper_gpgkey_e09422b3] | join(' ') }} | |
| {% endif %} |
🤖 Prompt for AI Agents
In templates/zypper.repo.j2 around lines 22 to 26, the gpgkey entries are split
across multiple physical lines which breaks parsing; replace the multiple gpgkey
lines with a single gpgkey= line that concatenates all possible key variables
separated by spaces, using Jinja to filter out empty values and join them (e.g.
build a list of the key variables, select non-empty entries, then join with a
space) so the repo file emits one logical gpgkey line for SUSE ≥ 15.
…sing `install-ssi.sh` script (#682) * Remove legacy installer package and unit files * Stop legacy installer systemd services and reload systemd graph * Remove configuration and installation of datadog-installer * Remove leftover from pr #624 installer/apm packages from pkg-redhat * Remove air-gapped installer doc and some installer-specific tests * First APM SSI iteration downloading and running the script * fix env placement + correct yaml * fix legacy installer removal task * Remove installer tests * Fix legacy installer removal conditionals * Only create install.json file if apm_instrumentation enabled is set * Use specific package built-in to avoid erroring out on AL2023 * Do not fail when trying to remove the package * Change logic for APM SSI to determine installation instead of weird skipping logic * Check apm status from installer + add .ansible to gitignore
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
tasks/agent-linux.yml (2)
13-16: CRITICAL: Variable naming inconsistency breaks system-probe version-gating.Lines 14–16 reference
datadog_major,datadog_minor, anddatadog_bugfix(the old variable names), but the PR renames these toagent_datadog_major,agent_datadog_minor, andagent_datadog_bugfix. Since these old variables are not defined, theagent_datadog_before_7610flag will always evaluate to false, breaking version-gated logic for system-probe and security-agent on versions below 7.61.0. This is a blocker that must be fixed before merge.Apply this diff to use the renamed variables:
- name: Set before 6/7.61.0 flag ansible.builtin.set_fact: - agent_datadog_before_7610: "{{ datadog_major is defined and datadog_minor is defined and datadog_bugfix is defined + agent_datadog_before_7610: "{{ agent_datadog_major is defined and agent_datadog_minor is defined and agent_datadog_bugfix is defined - and datadog_major | int < 8 - and (datadog_minor | int < 61) }}" + and agent_datadog_major | int < 8 + and (agent_datadog_minor | int < 61) }}"
53-87: CRITICAL: System-probe enablement conditions overlap, causing multiple set_fact executions.Lines 53–87 contain four "Set system probe enabled" tasks with overlapping
whenconditions. For Agent version 7.50.0:
- Line 57:
agent_datadog_before_7241 = false→ condition fails ✓- Line 66:
not agent_datadog_before_7241 = true→ executes ✗- Line 76:
not agent_datadog_before_7400 = true→ executes ✗- Line 87:
not agent_datadog_before_7610 = true→ executes ✗All three tasks (lines 66, 76, 87) execute, and the last one (line 87) overwrites the previous values. This violates the principle of idempotency and produces unpredictable results. The conditions must be mutually exclusive.
Fix the overlapping conditions by adding explicit lower-bound checks:
- name: Set system probe enabled (since 6/7.24.1) ansible.builtin.set_fact: agent_datadog_sysprobe_enabled: "{{ ((system_probe_config is defined and 'enabled' in (system_probe_config | default({}, true)) and system_probe_config['enabled']) or (network_config is defined and 'enabled' in (network_config | default({}, true)) and network_config['enabled'])) and agent_datadog_sysprobe_installed }}" - when: not datadog_skip_running_check and (not agent_datadog_before_7241) + when: not datadog_skip_running_check and agent_datadog_before_7400 and (not agent_datadog_before_7241) - name: Set system probe enabled (since 6/7.40.0) ansible.builtin.set_fact: agent_datadog_sysprobe_enabled: "{{ ((system_probe_config is defined and 'enabled' in (system_probe_config | default({}, true)) and system_probe_config['enabled']) or (network_config is defined and 'enabled' in (network_config | default({}, true)) and network_config['enabled']) or (service_monitoring_config is defined and 'enabled' in (service_monitoring_config | default({}, true)) and service_monitoring_config['enabled'])) and agent_datadog_sysprobe_installed }}" - when: not datadog_skip_running_check and (not agent_datadog_before_7400) + when: not datadog_skip_running_check and agent_datadog_before_7610 and (not agent_datadog_before_7400) - name: Set system probe enabled (since 6/7.61.0) ansible.builtin.set_fact: agent_datadog_sysprobe_enabled: "{{ ((system_probe_config is defined and 'enabled' in (system_probe_config | default({}, true)) and system_probe_config['enabled']) or (network_config is defined and 'enabled' in (network_config | default({}, true)) and network_config['enabled']) or (service_monitoring_config is defined and 'enabled' in (service_monitoring_config | default({}, true)) and service_monitoring_config['enabled'])) or (system_probe_other_config is defined and (system_probe_other_config is mapping | default({}, true)) and system_probe_other_config | length > 0) and agent_datadog_sysprobe_installed }}" - when: not datadog_skip_running_check and (not agent_datadog_before_7610) + when: not datadog_skip_running_check and (not agent_datadog_before_7610)tasks/pkg-debian.yml (4)
28-28: Fix stat mode comparison: use octal format, not string.Line 28 compares
agent_apt_keyring_file.stat.mode(an integer) against the string"0644". This comparison always fails, causing the keyring file to be recreated on every run.Apply this fix:
- when: not ansible_check_mode and (not agent_apt_keyring_file.stat.exists or not agent_apt_keyring_file.stat.mode == "0644") + when: not ansible_check_mode and (not agent_apt_keyring_file.stat.exists or not agent_apt_keyring_file.stat.mode == 0o644)
16-45: Update variable names to newagent_datadog_*naming convention.Several variables in this section still use the old
datadog_prefix and should follow the new naming scheme introduced in the PR:
- Lines 16, 18, 23:
datadog_apt_usr_share_keyring→agent_datadog_apt_usr_share_keyring- Line 49 (after fixing YAML):
datadog_apt_trusted_d_keyring→agent_datadog_apt_trusted_d_keyring- Line 32:
datadog_apt_default_keys→agent_datadog_apt_default_keys- Lines 39, 41, 43, 45:
datadog_apt_key_url_new→agent_datadog_apt_key_url_newExample:
- when: datadog_apt_key_url_new is not defined and not ansible_check_mode + when: agent_datadog_apt_key_url_new is not defined and not ansible_check_mode
48-56: Remove malformed YAML line that breaks task structure.Line 49 contains invalid YAML syntax (
keyring1,keyring2= {{ ... }}) that is not a valid Ansible task attribute. This line should be removed; the task structure is otherwise correct with src, dest, mode, and remote_src attributes.Apply this fix:
- name: Ensure keyring1 exists with same contents as keyring2 for older distro versions. - keyring1,keyring2= {{ datadog_apt_trusted_d_keyring, datadog_apt_usr_share_keyring }} ansible.builtin.copy:
72-81: Update repository variable names and fix type consistency.Lines 80-81 reference
datadog_agent6_apt_repoanddatadog_agent7_apt_repo, which should follow the new naming convention. Also, line 76 comparesitem.key(integer from dict keys) againstagent_datadog_agent_major_version | int; cast both to strings for clarity.Apply these fixes:
with_dict: - 6: "{{ datadog_agent6_apt_repo }}" - 7: "{{ datadog_agent7_apt_repo }}" + 6: "{{ agent_datadog_agent6_apt_repo }}" + 7: "{{ agent_datadog_agent7_apt_repo }}"And update the state condition:
- state: "{% if item.key == agent_datadog_agent_major_version | int and datadog_apt_repo | length == 0 %}present{% else %}absent{% endif %}" + state: "{% if item.key | string == agent_datadog_agent_major_version | string and agent_datadog_apt_repo | length == 0 %}present{% else %}absent{% endif %}"
🧹 Nitpick comments (6)
tasks/_agent-linux-macos-shared.yml (1)
126-129: Consider gating debug output to verbosity levels.The debug task at lines 126–129 prints the full install.json content unconditionally whenever it exists. For production deployments, consider adding
verbosity: 2to reduce log clutter.Apply this diff to make the debug task conditional:
- name: Debug print install.json content ansible.builtin.debug: msg: "install.json content : {{ install_json_value.content }}" + verbosity: 2 when: install_file.stat.existsREADME.md (1)
336-372: Minor: Add hyphen to "third party" compound adjective (line 337).Per Markdown best practices and grammar, "third party" should be "third-party" when used as a compound adjective modifying "integrations".
Apply this diff:
-##### Third party integrations~ +##### Third-party integrations~tasks/apm-single-step-instrumentation.yml (2)
38-45: Add timeout to prevent hanging on failed downloads.The
get_urltask should include atimeoutparameter to prevent indefinite hangs if the download server is unresponsive.Apply this diff:
- name: Download datadog-installer install-ssi.sh script ansible.builtin.get_url: url: "{{ datadog_installer_install_ssi_script_url }}" dest: /tmp/datadog-installer/install-ssi.sh mode: "0750" + timeout: 30 # Always overwrite the file to ensure we have the latest version. force: true register: install_ssi_script_result
47-52: Improve error handling for script execution; reconsider changed_when logic.Line 51's guard (
when: install_ssi_script_result.status_code == 200) is overly specific—get_urldoesn't setstatus_codeon successful downloads; it only fails if the download fails. Line 52'schanged_when: trueassumes the script always modifies state, which may not be accurate. Consider registering the result and checking for success more robustly.Apply this diff to improve error handling:
- name: Run datadog-installer install-ssi.sh script ansible.builtin.command: cmd: /tmp/datadog-installer/install-ssi.sh environment: "{{ datadog_apm_single_step_instrumentation_environment }}" - when: install_ssi_script_result.status_code == 200 - changed_when: true + register: ssi_script_result + changed_when: ssi_script_result.rc == 0 + failed_when: ssi_script_result.rc != 0tasks/pkg-redhat.yml (1)
87-95: GPG key list should be filtered to only include necessary keys per version.Lines 106–111 and similar blocks include all five GPG keys for every repository configuration (Agent 6, 7, custom), but not all keys are needed for all versions. For example, the E09422B3 key is only imported conditionally (lines 40, 46) based on minor version. Including unused keys in the repo configuration is redundant and may cause unnecessary warnings.
Consider filtering the gpgkey list to include only keys that are actually imported for the target version.
tasks/pkg-suse.yml (1)
80-86: Addforce: trueto key downloads for consistency with other distributions.Across pkg-redhat.yml and pkg-suse.yml, some get_url tasks for key downloads include
force: trueto ensure the latest version is always fetched, but lines 80–86 and 108–114 in pkg-suse.yml omit this parameter. For consistency and to ensure up-to-date keys are always used, addforce: trueto these tasks.Apply this diff:
- name: Download 20200908 key (Expires 2024) RPM key ansible.builtin.get_url: url: "{{ datadog_zypper_gpgkey_20200908 }}" dest: /tmp/DATADOG_RPM_KEY_20200908.public checksum: sha256:{{ datadog_zypper_gpgkey_20200908_sha256sum }} mode: '600' + force: true when: ansible_distribution_version|int >= 12 - name: Download 20280418 key (Expires 2028) RPM key ansible.builtin.get_url: url: "{{ datadog_zypper_gpgkey_20280418 }}" dest: /tmp/DATADOG_RPM_KEY_20280418.public checksum: "sha256:{{ datadog_zypper_gpgkey_20280418_sha256sum }}" mode: '600' + force: true when: ansible_distribution_version|int >= 12Also applies to: 108-114
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.circleci/config.yml(3 hunks).gitignore(1 hunks)README.md(16 hunks)defaults/main.yml(4 hunks)handlers/main.yml(1 hunks)tasks/_agent-linux-macos-shared.yml(1 hunks)tasks/agent-linux.yml(1 hunks)tasks/apm-single-step-instrumentation.yml(1 hunks)tasks/main.yml(1 hunks)tasks/pkg-debian.yml(1 hunks)tasks/pkg-redhat.yml(1 hunks)tasks/pkg-suse.yml(1 hunks)tasks/remove-legacy-installer.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🪛 Gitleaks (8.29.0)
tasks/pkg-debian.yml
[high] 38-38: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 40-40: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 42-42: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 44-44: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
defaults/main.yml
[high] 106-106: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 109-109: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 112-112: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 115-115: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 128-128: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 130-130: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 132-132: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 134-134: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 216-216: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 218-218: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 220-220: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 222-222: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
README.md
[style] ~91-~91: Consider shortening this phrase to just ‘whether’, unless you mean ‘regardless of whether’.
Context: ...og_apply_windows_614_fix` | Whether or not to download and apply file referenced b...
(WHETHER)
[uncategorized] ~92-~92: The operating system from Apple is written “macOS”.
Context: ...ren't running Datadog Agent 6.14.*.| | datadog_macos_user | The name...
(MAC_OS)
[uncategorized] ~93-~93: The operating system from Apple is written “macOS”.
Context: ...ults to ansible_user (macOS only).| | datadog_macos_download_url | Override...
(MAC_OS)
[uncategorized] ~309-~309: The operating system from Apple is written “macOS”.
Context: ... string. #### macOS When the variable datadog_macos_download_url is not set, the official ...
(MAC_OS)
[uncategorized] ~337-~337: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...remove`: Removes an integration. ##### Third party integrations [Datadog community][20] a...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[style] ~559-~559: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...rmation applies to versions of the role prior to 4.9.0. Since 4.9.0, the apt_key modul...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
🪛 markdownlint-cli2 (0.18.1)
README.md
91-91: Bare URL used
(MD034, no-bare-urls)
294-294: Bare URL used
(MD034, no-bare-urls)
302-302: Bare URL used
(MD034, no-bare-urls)
303-303: Bare URL used
(MD034, no-bare-urls)
313-313: Bare URL used
(MD034, no-bare-urls)
314-314: Bare URL used
(MD034, no-bare-urls)
315-315: Bare URL used
(MD034, no-bare-urls)
316-316: Bare URL used
(MD034, no-bare-urls)
326-326: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
607-607: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
625-625: Link and image reference definitions should be needed
Unused link or image reference definition: "2"
(MD053, link-image-reference-definitions)
🔇 Additional comments (18)
tasks/remove-legacy-installer.yml (1)
1-36: ✓ Solid best-effort legacy cleanup implementation.The use of
failed_when: falsethroughout is appropriate for removing legacy components that may not exist on all systems. The coverage of multiple systemd paths ensures compatibility across RHEL/Debian/SUSE distributions.tasks/_agent-linux-macos-shared.yml (1)
107-172: Install.json lifecycle management is sound but relies on implicit facts.The install.json management (lines 115–172) correctly handles both new and existing installations by parsing existing signatures or generating new UUIDs. The UUID fallback chain (cat /proc/sys/kernel/random/uuid → uuidgen) is resilient. However, line 172 creates the install.json only when
datadog_apm_instrumentation_enabledis defined—ensure all paths that depend on install.json account for this conditional creation.Please verify that install.json is created in all necessary contexts:
- Is
datadog_apm_instrumentation_enabledalways defined when install.json is needed?- If not, consider unconditionally creating install.json as part of the shared configuration path for consistency.
README.md (4)
5-18: ✓ Clear and helpful role vs. collection documentation.The distinction between the Datadog collection (datadog.dd) and standalone role (datadog.datadog) is well explained, with links to both Galaxy pages and a note on shared configuration variables.
49-97: ✓ Comprehensive role variables table with clear descriptions.The variables table is well-organized and includes all key configuration options. Mandatory fields (e.g.,
datadog_api_key) are clearly marked, and deprecation notices are included for removed variables (e.g.,datadog_yum_gpgkey).
208-254: ✓ System probe and NPM configuration documentation is accurate and well-structured.The updates reflect the shift from
system_probe_configtonetwork_configand introducesystem_probe_other_configfor additional configuration. The Windows NPM note (line 223) is helpful. Example configuration is clear and accurate.
536-553: ✓ Windows uninstallation guidance is practical and well-documented.The script to locate and uninstall the Datadog Agent on Windows is clear, with proper error handling for cases where the Agent is not installed.
tasks/pkg-redhat.yml (1)
116-158: ✓ Repository configuration guards are correct and mutually exclusive.The three yum_repository tasks (Agent 6, Agent 7, custom) are properly guarded:
- Agent 6:
agent_datadog_agent_major_version | int == 6anddatadog_yum_repo | length == 0- Agent 7:
agent_datadog_agent_major_version | int == 7anddatadog_yum_repo | length == 0- Custom:
datadog_yum_repo | length > 0These conditions ensure exactly one repository is configured.
handlers/main.yml (2)
10-12: ✓ Service facts gathering and restart logic correctly gate on OS type.The service facts are gathered only on Linux/macOS (excluding Windows), which avoids unnecessary operations on Windows. The main restart handler then checks
ansible_facts.servicesfor the presence and state of the exp service, preventing conflicts between experimental and production agents. This is a clean approach to handling multiple service variants.Also applies to: 14-20
22-38: ✓ Excellent use of include_tasks to avoid unnecessary collection dependencies.The Windows and macOS restart handlers use
include_tasksto load OS-specific logic only when needed, avoiding the requirement for theansible.windowscollection on Linux hosts. This is the correct pattern for modular, platform-specific Ansible code. The comments clearly explain the rationale.tasks/agent-linux.yml (1)
99-156: ✓ Service lifecycle management is well-structured and idempotent.The main agent service orchestration correctly uses
ansible.builtin.servicewith explicitenabledandusesemantics. The guards prevent operations on disabled/check modes, and the exp service check avoids conflicts with experimental agents. The inclusion of older agent services (datadog-agent-process, datadog-agent-trace) ensures clean lifecycle management across versions.tasks/pkg-suse.yml (1)
52-58: Fix incomplete task definition: add missingurl:keyword.Line 53–55 is missing the
url:keyword before the URL value. This will cause a YAML parsing error.Apply this diff to fix the syntax:
- name: Download E09422B3 key (Expires 2022) RPM key ansible.builtin.get_url: - url: "{{ datadog_zypper_gpgkey_e09422b3 }}" + url: "{{ datadog_zypper_gpgkey_e09422b3 }}" dest: /tmp/DATADOG_RPM_KEY_E09422B3.public checksum: sha256:{{ datadog_zypper_gpgkey_e09422b3_sha256sum }} mode: '600' when: ansible_distribution_version|int >= 12 and agent_datadog_minor is defined and agent_datadog_minor | int < 36Wait, looking at the code again, line 54 shows
url:is present. Let me re-check. The issue is that lines 52-58 in the code block show:- name: Download E09422B3 key (Expires 2022) RPM key ansible.builtin.get_url: url: "{{ datadog_zypper_gpgkey_e09422b3 }}" # line 54 dest: /tmp/DATADOG_RPM_KEY_E09422B3.public # line 55 checksum: sha256:... # line 56 - extra space?Actually, re-reading the annotation, lines 52-58 appear correctly formatted. Let me reconsider.
Actually, I see line 54 doesn't have "url:" label in the original:
- name: Download E09422B3 key (Expires 2022) RPM key ansible.builtin.get_url: url: "{{ datadog_zypper_gpgkey_e09422b3 }}" # This IS line 54 in the display dest: /tmp/DATADOG_RPM_KEY_E09422B3.publicWait, looking more carefully at the formatting, I see the code is properly structured. Let me re-examine if there's actually an issue...
Actually, the code provided shows the full task and it looks correct. Let me revise my comment.
Likely an incorrect or invalid review comment.
.circleci/config.yml (2)
88-88: Approve: parameter threading and jinja2_native propagation.The addition of
jinja2_nativeparameter (lines 21-23) and threading it throughinstall_agentcommand invocation (line 88) is correct. The YAML environment variable setup at line 28 (ANSIBLE_JINJA2_NATIVE="<<parameters.jinja2_native>>") properly passes the value to ansible-playbook.
173-205: Approve: macOS job structure and Python setup.The new
test_install_macosjob correctly:
- Installs Rosetta 2 for ARM64 compatibility (lines 193-194)
- Installs Python 3.11 via Homebrew (lines 196-197)
- Uses the full path to installed Python in the test command (line 203:
/opt/homebrew/bin/python3.11)- Properly passes parameters through the command invocation
The setup aligns with the explicit Python version in the
install_agentcommand invocation at line 28, which expects a full path like/usr/bin/python3or/opt/homebrew/bin/python3.11.tasks/main.yml (3)
2-8: Approve: ansible.builtin.setup with gather_subset parameter.The explicit ansible.builtin.setup module invocation with gather_subset parameter is correct. The comment on line 5 correctly notes that the full prefix is necessary to avoid Ansible picking the wrong module on Windows systems.
17-20: Approve: API key validation for managed config.The fail task correctly enforces
datadog_api_keypresence whendatadog_manage_configis enabled, preventing silent configuration failures. This is a good defensive check.
93-112: Approve: APM Single Step Instrumentation validation.The implementation correctly:
- Sets a fact to determine if instrumentation should be installed (lines 95-100)
- Validates the configuration value against allowed options (lines 102-107)
- Conditionally includes the instrumentation task (lines 109-112) with check_mode guard
The inline validation prevents misconfiguration errors early.
defaults/main.yml (2)
2-2: Approve: comprehensive role expansion for version 5.4.1.The defaults file reflects a well-structured expansion of the Datadog Ansible role:
Version and feature flags (lines 2-8): Version 5.4.1 with explicit
datadog_enabledanddatadog_manage_configflags provide clear configuration control.System probe and integration surface (lines 14-17, 22-35): New config blocks for system-probe, network monitoring, and service monitoring expand the role's capabilities.
Package flavor (line 65): Introduction of
datadog_agent_flavorallows support for alternative agent builds (e.g., IoT Agent).APM instrumentation (lines 182-198): Structured APM configuration with optional library pinning supports host and Docker injection modes.
macOS support (lines 237-265): Comprehensive macOS additions including DMG URLs, user/service paths, and architecture-specific downloads align with the PR's macOS expansion goals.
Repository and key management (lines 208-235): Centralized GPG key definitions and repository configuration with
signed-byoptions modernize package verification.Also applies to: 65-65, 182-198, 237-265
226-227: Verify defaults align with updated variable names in task files.The repository definitions at lines 226-227 (apt), 230-231 (yum), and 234-235 (zypper) use the
agent_datadog_agent6_apt_repo,agent_datadog_agent6_yum_repo, andagent_datadog_agent6_zypper_reponaming. Verify that:
- All task files (pkg-debian.yml, pkg-redhat.yml, pkg-suse.yml, pkg-windows.yml) reference these exact variable names.
- The variables in pkg-debian.yml have been updated (past review flagged inconsistencies; current code still shows old names).
This is a precondition for the role to function correctly after the migration.
Also applies to: 230-231, 234-235
| ansible_version: ["2_10", "3_4", "4_10", "9_4"] | ||
| agent_version: ["6", "7"] | ||
| jinja2_native: ["true", "false"] | ||
| os: ["rocky8"] | ||
| python: ["python3"] |
There was a problem hiding this comment.
❓ Verification inconclusive
Ansible 9.4 incompatible with Rocky Linux 8's default Python 3.6.
Line 345 tests Ansible 9.4 with python: ["python3"] on rocky8, but Ansible 9 requires Python 3.10 or newer. Rocky Linux 8's python3 link defaults to Python 3.6.8. This combination will fail at runtime.
Options:
- Upgrade to Python 3.10+: specify
python: ["python3.10"]orpython: ["python3.11"]for this matrix. - Remove Ansible 9.4 from rocky8 tests if Python 3.10+ is unavailable on that distribution.
- Use a newer Rocky version (e.g., rocky9) which has Python 3.9+ available.
I'll verify the claims in the review comment by checking the Ansible compatibility requirements and examining the actual configuration file.Now let me search for Ansible 9.4 specific requirements:
Now let me examine the actual CircleCI config file to verify the specific issue at lines 345-349:
Based on my web search verification, I have confirmed all the key facts in the review comment:
-
Ansible 9 removed support for Python 3.9 and requires Python 3.10 or newer on the controller
-
Rocky Linux 8's python3 link defaults to Python 3.6.8, which takes precedence over other installed versions
The review comment accurately identifies a critical compatibility issue that will cause runtime failures when testing Ansible 9.4 with the default Rocky Linux 8 Python. The suggestions provided are all valid and appropriate.
Ansible 9.4 incompatible with Rocky Linux 8's default Python 3.6.
Line 345 tests Ansible 9.4 with python: ["python3"] on rocky8, but Ansible 9 requires Python 3.10 or newer on the control node. Rocky Linux 8's python3 defaults to Python 3.6.8. This combination will fail at runtime.
Options:
- Upgrade to Python 3.10+: specify
python: ["python3.10"]orpython: ["python3.11"]for this matrix. - Remove Ansible 9.4 from rocky8 tests if Python 3.10+ is unavailable on that distribution.
- Use a newer Rocky version (e.g., rocky9) which has Python 3.9+ available.
🤖 Prompt for AI Agents
.circleci/config.yml lines 345-349: the matrix tests Ansible 9.4 on rocky8 with
python: ["python3"], but Ansible 9 requires Python >=3.10 while Rocky Linux 8's
python3 points to 3.6.8, so update the matrix to ensure Python >=3.10 for
Ansible 9.4 (e.g., set python: ["python3.10"] or ["python3.11"] for entries that
use ansible_version "9_4"), or remove the "9_4" ansible entry for rocky8, or
change the os to a newer distribution (rocky9) that provides Python 3.10+; apply
the chosen change consistently in the matrix so CI runs use a compatible Python
for Ansible 9.4.
| set_fact: | ||
| datadog_remove_custom_repo_file: "{{ datadog_repo_file_contents != datadog_apt_repo }}" | ||
| ansible.builtin.set_fact: | ||
| agent_datadog_remove_custom_repo_file: "{{ agent_datadog_repo_file_contents != datadog_apt_repo }}" |
There was a problem hiding this comment.
Update datadog_apt_repo references to new naming convention.
Lines 100, 109, 114, and 117 use datadog_apt_repo, which should be agent_datadog_apt_repo per the PR's variable migration:
- when: (datadog_apt_repo | length == 0) or agent_datadog_remove_custom_repo_file and (not ansible_check_mode)
+ when: (agent_datadog_apt_repo | length == 0) or agent_datadog_remove_custom_repo_file and (not ansible_check_mode) repo: "{{ datadog_apt_repo }}"
+ repo: "{{ agent_datadog_apt_repo }}"- when: (datadog_apt_repo | length > 0) and (not ansible_check_mode)
+ when: (agent_datadog_apt_repo | length > 0) and (not ansible_check_mode)Also applies to: 109-109, 114-114, 117-117
🤖 Prompt for AI Agents
In tasks/pkg-debian.yml around lines 100, 109, 114 and 117, update uses of the
old variable datadog_apt_repo to the new name agent_datadog_apt_repo; replace
each occurrence (including in expressions like
agent_datadog_remove_custom_repo_file) so the playbook references the migrated
variable, then run a lint/ansible-playbook --syntax-check to confirm no other
references remain.
…all of Datadog Agent (#683) * [AGENTONB-2680][Windows] Provide users a way to skip and force reinstall of Datadog Agent * Change how we uninstall by looking for the installed productcode instead of msi to allow downgrades * clarify the precedence of the option * whitespace trim
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tasks/pkg-windows.yml (2)
2-20: Variable naming convention inconsistency throughout Windows flow.The file mixes
datadog_windows_*(legacy) andagent_datadog_*(new) naming styles. Input flags use the old convention, then map to the new convention internally. This pattern repeats throughout the file (e.g.,datadog_windows_614_fix_script_urlis later referenced in lines 24–27, whileagent_dd_download_urlis used in lines 45, 56). While the mapping is explicit, this hybrid approach complicates variable tracking and increases maintenance burden.Consider:
- Standardizing on a single naming convention across the role (either commit to
agent_datadog_*throughout or stick withdatadog_windows_*).- Documenting the mapping clearly if both styles must coexist.
- Adding a centralized variable mapping layer if this pattern appears elsewhere.
110-120: Password handling approach is reasonable but has limitations.The password is appended to
agent_win_install_argswithno_log: trueon the task. This prevents the task itself from being logged but does not protect the variable if it's:
- Referenced or inspected by other tasks
- Dumped in error messages or debug output
- Included in playbook/facts output
For maximum security, consider:
- Storing the password separately from
agent_win_install_argsand injecting it only at the final install command.- Documenting that the
agent_win_install_argsvariable must be treated as sensitive throughout subsequent tasks.- Using Ansible's vault or external secret management if this role will run in sensitive environments.
Current approach is acceptable for typical use cases but should be documented.
defaults/main.yml (1)
76-135: Repository and GPG key management is comprehensive but shows DRY violations.The configuration provides good forward planning with multiple GPG keys tracked by expiration date (2022, 2024, 2028, 2033). However,
datadog_yum_gpgkey_*definitions (lines 103–116) are duplicated identically indatadog_zypper_gpgkey_*(lines 126–134).Consider:
- Extracting common key definitions to reduce duplication.
- Adding comments explaining each key's expiration date and rotation plan.
- Documenting the process for adding/retiring GPG keys.
README.md (4)
53-100: Variable documentation is comprehensive but has minor grammar issues.The table clearly documents new variables with platform tags and external documentation references. However, Line 103 has a grammar issue: "first level key" should be "first-level key" (hyphenated as a compound modifier). Additionally, verify that example configurations in the "Playbooks" section (lines ~388–463) accurately reflect these variable names and usage patterns.
Verify that all code examples later in the README use the correct variable names and structure, particularly:
- Line 461:
network_configexample- Line 95–96:
datadog_macos_userdefault toansible_user(confirm this works correctly in macOS tasks)
101-225: Integration and system probe documentation clearly explains new configuration structure.The restructuring from
system_probe_configto separatenetwork_config,runtime_security_config,service_monitoring_config, andsystem_probe_other_configis well-explained with clear examples. The new Custom Python Checks subsection (lines 141–157) and Autodiscovery section provide good guidance. The Windows NPM note (line 225) properly flags that NPM is optional and may require reinstall.Minor grammar issue at line 339: "Third party integrations" should be "third-party integrations" (hyphenated).
283-320: Repository and version documentation is comprehensive and well-organized.The addition of Windows and macOS repository sections with per-major-version defaults, architecture-aware URLs, and clear tables helps users understand installation sources across platforms.
Minor formatting issue: Lines 296, 304–305, and 315–318 contain bare URLs that should be wrapped in markdown link syntax
[text](url)for consistency and validation (flagged by markdownlint-cli2 as MD034).
538-625: Windows uninstallation and troubleshooting sections are comprehensive.The new Uninstallation section (lines 538–555) provides helpful guidance for Windows users. The Troubleshooting section appropriately marks deprecated information with version numbers (e.g., "versions of the role prior to 4.9.0," "version 5+") and explains critical issues like the Windows 6.14 Agent bug and missing API key requirement.
Minor issues:
- Line 609: Fenced code block should specify language (e.g.,
```jsonor```text) (flagged by MD040).- Line 627: Unused link reference
[2]should be removed (flagged by MD053).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(16 hunks)defaults/main.yml(4 hunks)tasks/pkg-windows.yml(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~91-~91: ‘Whether or not’ might be wordy. Consider a shorter alternative.
Context: ...og_apply_windows_614_fix` | Whether or not to download and apply file referenced b...
(EN_WORDINESS_PREMIUM_WHETHER_OR_NOT)
[uncategorized] ~94-~94: The operating system from Apple is written “macOS”.
Context: ...indows_skip_install(Windows only).| |datadog_macos_user` | The name...
(MAC_OS)
[uncategorized] ~95-~95: The operating system from Apple is written “macOS”.
Context: ...ults to ansible_user (macOS only).| | datadog_macos_download_url | Override...
(MAC_OS)
[grammar] ~103-~103: Use a hyphen to join words.
Context: ... the datadog_checks section. The first level key is the name of the check, and ...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~311-~311: The operating system from Apple is written “macOS”.
Context: ... string. #### macOS When the variable datadog_macos_download_url is not set, the official ...
(MAC_OS)
[grammar] ~339-~339: Use a hyphen to join words.
Context: ...e`: Removes an integration. ##### Third party integrations [Datadog community][...
(QB_NEW_EN_HYPHEN)
[style] ~561-~561: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...rmation applies to versions of the role prior to 4.9.0. Since 4.9.0, the apt_key modul...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
🪛 markdownlint-cli2 (0.18.1)
README.md
91-91: Bare URL used
(MD034, no-bare-urls)
296-296: Bare URL used
(MD034, no-bare-urls)
304-304: Bare URL used
(MD034, no-bare-urls)
305-305: Bare URL used
(MD034, no-bare-urls)
315-315: Bare URL used
(MD034, no-bare-urls)
316-316: Bare URL used
(MD034, no-bare-urls)
317-317: Bare URL used
(MD034, no-bare-urls)
318-318: Bare URL used
(MD034, no-bare-urls)
328-328: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
609-609: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
627-627: Link and image reference definitions should be needed
Unused link or image reference definition: "2"
(MD053, link-image-reference-definitions)
🔇 Additional comments (11)
tasks/pkg-windows.yml (4)
35-46: Gate logic for version handling is well-structured.The conditional includes for latest vs. pinned versions are mutually exclusive and properly gated. No concerns with this section.
54-60: MSI download and result registration look correct.The variable
agent_download_msi_resultis properly registered and consistently referenced in subsequent tasks (lines 134, 141, 143).
62-91: Product ID lookup logic is defensive and comprehensive.The PowerShell script properly handles both 64-bit and 32-bit registry paths, gracefully handles missing paths, and deduplicates results. The caching logic correctly filters for non-empty matches before setting the fact. The
changed_when: falseandcheck_mode: falsesettings are appropriate here.Verify that line 125 (uninstall loop) safely handles the case where
agent_datadog_windows_installed_product_idsmay not be defined if no product IDs were found or if force reinstall was not set.
121-143: Uninstall, install, and cleanup logic is well-gated and consistent.Uninstall runs only when force-reinstalling, install uses registered download result properly, and cleanup checks for successful download before removing temporary files. All gates are appropriate.
defaults/main.yml (5)
2-48: Version bump and new basic flags are appropriate.The addition of
datadog_enabled,datadog_manage_config, and check management flags provides fine-grained control. New macOS binary path and integration user variables complete platform coverage consistently with existing Linux/Windows patterns.
152-188: Windows installation defaults are comprehensive with good control flags.The MSI URLs, fix script handling, and skip/force-reinstall flags provide appropriate control. However, note that
win_install_argsdefaults to a single space (line 188). Verify in tasks that this is safely concatenated—if other args are appended without care, spacing could be incorrect.Verify that
win_install_args(line 188) is correctly used in task concatenation, particularly when the password arg is appended in tasks/pkg-windows.yml line 117.
190-207: Advanced installer and APM configuration variables are well-structured.The addition of APM instrumentation, remote updates, and installer customization options provides the flexibility needed for advanced scenarios. Default for
datadog_apm_instrumentation_librariesas an empty list is appropriate—users enable via configuration.
216-243: Internal repository defaults are well-structured with modern security practices.The use of
signed-bywith APT keyrings, architecture-aware repo URLs for yum/zypper, and clear "internal use only" comments are all appropriate. Architecture substitution properly usesansible_facts.architecture.
245-272: macOS defaults and Ansible facts configuration are well-designed.Architecture-aware URLs for Agent 7.70+ (separate arm64/x86_64 packages), standard macOS paths, and minimal facts gathering (
!all, !any, min, env) are all appropriate. Service name, plist file paths, and log directories follow macOS conventions correctly.README.md (2)
1-48: Documentation introduction and prerequisites are clear and comprehensive.The distinction between Ansible role and collection channels is well-explained, prerequisite for
ansible.windowscollection is clearly documented with installation instructions, and the API key requirement is appropriately explained with a workaround option.
388-463: Playbook examples are clear and appropriately use updated variables.The main sample playbook correctly uses
network_configand demonstrates integration installation. Examples for Agent v6, site configuration, and Windows-specific deployments provide good coverage for different scenarios.
* [AGENTONB-2680][Windows] Provide users a way to skip and force reinstall of Datadog Agent * Change how we uninstall by looking for the installed productcode instead of msi to allow downgrades * [AGENTONB-2711] Allow Agent downgrades on Windows by uninstalling first
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tasks/pkg-windows-opts.yml (1)
60-85: Validation logic is correct but hard to read.The path validation properly handles upgrade scenarios by comparing registry values against provided config/install directories. However, the multi-line when conditions are difficult to parse. Consider extracting the conditions to variables for clarity.
Example refactor for readability:
+ - name: Set config validation conditions + ansible.builtin.set_fact: + agent_config_mismatch: >- + (agent_config_root_from_registry.exists) and + (datadog_windows_config_files_dir | length > 0) and + (agent_config_root_from_registry.value | regex_replace('\\\\$','') | lower != + datadog_windows_config_files_dir | lower) + - name: Validate config path ansible.builtin.fail: msg: "Incompatible configuration option {{ agent_config_root_from_registry.value }} != {{ datadog_windows_config_files_dir }}" - when: ( (agent_config_root_from_registry.exists) and (datadog_windows_config_files_dir | length > 0 ) and (agent_config_root_from_registry.value - | regex_replace('\\\\$','') | lower != datadog_windows_config_files_dir | lower ) ) + when: agent_config_mismatch
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tasks/parse-version-windows.yml(1 hunks)tasks/pkg-windows-opts.yml(1 hunks)
🔇 Additional comments (10)
tasks/pkg-windows-opts.yml (7)
19-35: PowerShell feature discovery logic looks sound.The script correctly discovers installed Datadog Agent features via COM/registry inspection, filters appropriately, and outputs results for consumption by downstream tasks.
42-55: Registry queries are properly structured.The use of
win_reg_statto query ConfigRoot and InstallPath is appropriate, and registering both the result and.existsflag enables proper conditional handling downstream.
87-108: Directory setup correctly handles upgrade and new install paths.The logic properly distinguishes between providing custom paths via install arguments and detecting existing registry-based paths during upgrades.
111-114: NPM flag setup is correct.The use of
ADDLOCAL=MainApplication,NPMsyntax is appropriate for Windows Installer feature selection when sysprobe is enabled.
129-163: Downgrade detection logic is sound and well-guarded.The three-part version comparison correctly identifies downgrades, properly guards against undefined version variables, and appropriately triggers both skip_install disable and force_reinstall enable for downgrade scenarios.
10-12: The repository clone operation failed, preventing me from searching the codebase for thewin_install_argsvariable definition. Without access to the repository structure, I cannot verify whether:
win_install_argsis defined in defaults files- The variable is set in earlier tasks or playbooks
- The variable scope and execution order are correct
Since the verification could not be completed and the information remains inconclusive, I'll provide the rewritten review comment with this limitation noted:
Verify
win_install_argsvariable is defined before this task execution.This task initializes
agent_win_install_argsfromwin_install_args(line 12), but the source variable's origin cannot be confirmed due to verification limitations. Ensurewin_install_argsis defined in defaults, vars files, or earlier in the playbook execution flow before this task runs.
117-127: I'm unable to clone the repository in the sandbox environment to verify the concern. Let me provide the rewritten review comment based on the analysis I can perform with the provided code snippet:
Verify the logic of the when condition that checks
agent_datadog_skip_installbefore setting it tofalse.The when condition (line 126) checks if
agent_datadog_skip_installistrue, but the task body (line 119) sets it tofalse. This appears contradictory and suggests one of the following:
- The condition should check
not agent_datadog_skip_installinstead ofagent_datadog_skip_install- The variable semantics are inverted from their name (i.e.,
skip_install: truemeans "do install")- There is a logical error in the condition
Confirm the intended behavior: should this task execute when the agent installation should be skipped, or when it should not be skipped? Review the preceding tasks to understand what state
agent_datadog_skip_installhas when this task is reached.tasks/parse-version-windows.yml (3)
33-38: Safe fallback handling for version component facts.The use of
default('', true)on lines 35-37 provides a graceful fallback if regex parsing fails or returns incomplete results. The conversion to string is explicit and appropriate. The logic here handles the edge case where regex_search returns fewer than three groups.
23-31: Theregex_searchsyntax is correct and produces the expected list output.The Ansible documentation confirms that
regex_searchwith multiple named group backreferences ('\\g<name>') returns a list of the captured values. The syntax in the code—regex_search(installed_agent_regexp, '\\g<major>', '\\g<minor>', '\\g<bugfix>')—is the standard Ansible approach and will correctly produce a list['major_value', 'minor_value', 'bugfix_value']that can be accessed via.0,.1,.2indexing.
3-21: Based on my verification, I have a critical finding: The Datadog Agent's Windows DisplayVersion in the registry is always 4 components (major.minor.patch.build), e.g., "7.69.1.0". This means the edge case described in the original review—where the version string has fewer than 3 components—will never occur in practice.The original concern about malformed output like "7.43." was theoretically valid for arbitrary version strings, but is not applicable to the Datadog Agent, which always provides exactly 4 numeric components in the registry's DisplayVersion field.
Additionally, I confirmed that:
- The Ansible
regex_search()syntax with multiple named groups using'\\g<name>'is correct and valid- The code gracefully handles any failures with
failed_when: false
The review comment's primary concern is not applicable to Datadog Agent versioning; the code is adequate as-is.
The PowerShell script correctly parses the guaranteed 4-part Datadog Agent version string from the Windows registry and outputs the 3-part semantic version. The subsequent regex parsing and fact assignment work as intended. No changes are required.
| - name: Set DD Username Arg | ||
| set_fact: | ||
| win_install_args: "{{ win_install_args }} DDAGENTUSER_NAME={{ datadog_windows_ddagentuser_name }}" | ||
| when: datadog_windows_ddagentuser_name | length > 0 | ||
| ansible.builtin.set_fact: | ||
| agent_win_install_args: "{{ win_install_args }} DDAGENTUSER_NAME={{ datadog_windows_ddagentuser_name }}" | ||
| when: datadog_windows_ddagentuser_name | default('', true) | length > 0 |
There was a problem hiding this comment.
Critical: Inconsistent variable reference breaks DD username argument.
Line 16 references win_install_args instead of agent_win_install_args. This breaks the DD username argument since later tasks (lines 88–89, 93–94, 112–113) all append to agent_win_install_args, which won't include the username set here.
Apply this diff to fix the variable reference:
- name: Set DD Username Arg
ansible.builtin.set_fact:
- agent_win_install_args: "{{ win_install_args }} DDAGENTUSER_NAME={{ datadog_windows_ddagentuser_name }}"
+ agent_win_install_args: "{{ agent_win_install_args }} DDAGENTUSER_NAME={{ datadog_windows_ddagentuser_name }}"
when: datadog_windows_ddagentuser_name | default('', true) | length > 0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Set DD Username Arg | |
| set_fact: | |
| win_install_args: "{{ win_install_args }} DDAGENTUSER_NAME={{ datadog_windows_ddagentuser_name }}" | |
| when: datadog_windows_ddagentuser_name | length > 0 | |
| ansible.builtin.set_fact: | |
| agent_win_install_args: "{{ win_install_args }} DDAGENTUSER_NAME={{ datadog_windows_ddagentuser_name }}" | |
| when: datadog_windows_ddagentuser_name | default('', true) | length > 0 | |
| - name: Set DD Username Arg | |
| ansible.builtin.set_fact: | |
| agent_win_install_args: "{{ agent_win_install_args }} DDAGENTUSER_NAME={{ datadog_windows_ddagentuser_name }}" | |
| when: datadog_windows_ddagentuser_name | default('', true) | length > 0 |
🤖 Prompt for AI Agents
In tasks/pkg-windows-opts.yml around lines 14 to 17, the set_fact uses the wrong
variable name (win_install_args) so the DDAGENTUSER_NAME is not appended to the
shared agent_win_install_args used later; update the set_fact to append
DDAGENTUSER_NAME to agent_win_install_args instead of win_install_args so
subsequent tasks that extend agent_win_install_args include the username
(preserve the when condition and formatting).
* [AGENTONB-2680][Windows] Provide users a way to skip and force reinstall of Datadog Agent * Change how we uninstall by looking for the installed productcode instead of msi to allow downgrades * [AGENTONB-2711] Allow Agent downgrades on Windows by uninstalling first * Do not install/restart Agent when remote update is happening * Remove trailing space * Init remote update flag to false to simplify approach
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tasks/pkg-windows-opts.yml (1)
14-17: 🔴 CRITICAL: Unresolved bug—inconsistent variable reference breaks argument accumulation.This issue was flagged in past review and remains unfixed.
Line 16 references
win_install_args(the base input) instead ofagent_win_install_args(the accumulator). This breaks consistency and may cause issues if additional setup tasks are added between line 12 and line 16, or if line 16 is conditionally executed after other modifications.All subsequent append operations (lines 89, 94, 113) correctly append to
agent_win_install_args. Line 16 should do the same for consistency and safety.Apply this diff to fix the variable reference:
- name: Set DD Username Arg ansible.builtin.set_fact: - agent_win_install_args: "{{ win_install_args }} DDAGENTUSER_NAME={{ datadog_windows_ddagentuser_name }}" + agent_win_install_args: "{{ agent_win_install_args }} DDAGENTUSER_NAME={{ datadog_windows_ddagentuser_name }}" when: datadog_windows_ddagentuser_name | default('', true) | length > 0
🧹 Nitpick comments (1)
tasks/agent-win.yml (1)
27-28: Complex filter chains—ensure Windows path handling is robust.The filter chains using
win_dirname,win_basename, and regex replacements across multiple lines are complex. Verify:
- These filters exist in the Jinja2 environment (or are Ansible built-ins)
- Paths with backslashes and dots are handled correctly (e.g.,
C:\path\check.d\conf.yaml→check)- Edge cases (empty paths, malformed extensions) don't cause silent failures
Consider breaking these into intermediate variables for readability:
paths: "{{ agent_datadog_conf_paths.files | map(attribute='path') | select('match', '.*\\conf\\.yaml$') | list }}" dirnames: "{{ paths | map('win_dirname') | map('win_basename') | list }}" checks: "{{ dirnames | map('regex_replace', '^(.*).d$', '\\1') | list }}"Also applies to: 39-40, 51-52
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
handlers/main-win.yml(1 hunks)tasks/agent-win.yml(1 hunks)tasks/main.yml(1 hunks)tasks/pkg-windows-opts.yml(1 hunks)tasks/pkg-windows.yml(2 hunks)tasks/windows_remote_update_check.yml(1 hunks)
🔇 Additional comments (14)
tasks/windows_remote_update_check.yml (1)
1-27: LGTM!The remote update detection logic is sound. The PowerShell CIM query correctly identifies ongoing updates, and the flag initialization/setting/messaging flow integrates well with the downstream handler and task logic.
handlers/main-win.yml (1)
1-11: LGTM!The Windows handler-like task is well-documented and appropriately conditioned. The
force_dependent_servicesflag and thedatadog_remote_update_in_progressgate are correct safeguards for Windows service restarts.tasks/agent-win.yml (2)
18-18: Verify variable registration consistency.Lines 18 and 61 register
agent_datadog_conf_paths, but line 18 is inside a conditional and line 61 references it unconditionally. Ensure that any downstream task referencingagent_datadog_conf_pathsis also gated by the same condition or provides a default.[scratchpad_end] -->
Also applies to: 61-61
88-95: LGTM!The trace and process agent service management with the
datadog_remote_update_in_progressgate is correct. Thestart_mode: manualpreserves their enabled state without forcing immediate startup.tasks/main.yml (3)
17-20: LGTM!The Windows remote update check include is correctly conditioned and positioned before the Windows install tasks. This ensures the flag is initialized before being checked in other Windows flows.
53-56: Verify that datadog_rpm_remove_keys loop is properly initialized.Line 56 loops over
datadog_rpm_remove_keys, which is not initialized in this file. Ensure:
- This variable is defined in
defaults/main.ymlwith a safe default (e.g., empty list)- The loop will not fail if the variable is undefined
If the variable may be undefined, add a fallback:
loop: "{{ datadog_rpm_remove_keys | default([], true) }}"
108-127: Verify APM instrumentation validation and integration.The APM Single Step Instrumentation logic introduces three separate blocks (lines 108-127):
- Determining if instrumentation should be installed (lines 108-115)
- Explicit validation for invalid values (lines 117-122)
- Inclusion of instrumentation tasks (lines 124-127)
Verify:
- The
apm-single-step-instrumentation.ymlinclude file exists and is properly structured.- If
datadog_apm_instrumentation_enabledis not set, the flow gracefully skips (which it does).- The three blocks are sequenced correctly (determine → validate → include).
Otherwise, the logic is sound.
tasks/pkg-windows-opts.yml (4)
19-35: LGTM!The Windows Installer feature discovery via PowerShell is robust. It safely handles missing registry keys and filter results to installed features only. The
changed_when: falseis appropriate for a read-only discovery task.
87-95: LGTM!The directory creation and permission setting logic for custom Program Files and Config Files directories is sound. Conditions and formatting are correct.
100-108: LGTM!The config root resolution logic correctly prioritizes explicit configuration over registry detection, and the regex to strip trailing backslashes is appropriate.
129-148: LGTM!The downgrade detection logic is comprehensive, correctly comparing major, minor, and bugfix versions in a logical order. The version comparison chain is well-structured.
tasks/pkg-windows.yml (3)
28-34: Verify that included version tasks set agent_dd_download_url.Lines 28 and 32 include
win_agent_latest.ymlandwin_agent_version.yml, which are not provided in this review. These tasks must setagent_dd_download_url, which is referenced on line 49. Verify:
- Both include files exist and are syntactically correct.
- Both files set
agent_dd_download_urlunder all relevant execution paths.- No code paths skip setting
agent_dd_download_urlbefore line 49.
103-105: Verify Jinja2 ternary syntax for conditional password display.Line 105 uses a complex Jinja2 ternary condition to conditionally append
DDAGENTUSER_PASSWORD=<REDACTED>to the debug message. Verify the syntax is correctly recognized by Ansible:msg: "{{ agent_win_install_args }}{% if datadog_windows_ddagentuser_password | default('', true) | length > 0 %} DDAGENTUSER_PASSWORD=<REDACTED>{% endif %}"This should work, but confirm that the filter chain and conditional render as expected in your Ansible version.
114-123: LGTM!The uninstall logic correctly gates the uninstall on the
agent_datadog_force_reinstallflag and verifies that product IDs are available. The loop and conditions are sound.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
CHANGELOG.md (1)
18-18: Multiple spelling and typographical errors persist from previous reviews.These issues were previously flagged but remain unfixed in the current code:
- Line 18: "MacOS" → "macOS" (Apple OS branding)
- Line 30: "notd" → "not" (typo)
- Line 31: "IMROVEMENT" → "IMPROVEMENT" (typo)
- Line 62: "MacOS" → "macOS" (Apple OS branding)
- Line 95: "instrumation" → "instrumentation" (typo)
Apply these diffs to fix all spelling errors:
-* [IMPROVEMENT] Simplify MacOS version architecture [#677] +* [IMPROVEMENT] Simplify macOS version architecture [#677]-* [IMPROVEMENT] Only gather facts and services if notd done already [#659] [Thanks @POIFischbacher] +* [IMPROVEMENT] Only gather facts and services if not done already [#659] [Thanks @POIFischbacher]-* [IMROVEMENT] [CI] Remove role prefix lint warnings [#660] +* [IMPROVEMENT] [CI] Remove role prefix lint warnings [#660]-* [IMPROVEMENT] [CI] Fix MacOS CI [#630] +* [IMPROVEMENT] [CI] Fix macOS CI [#630]-* [FEATURE] creating install.json file related to apm single step instrumation [#572] +* [FEATURE] creating install.json file related to apm single step instrumentation [#572]Also applies to: 30-30, 31-31, 62-62, 95-95
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(4 hunks)defaults/main.yml(4 hunks)
🧰 Additional context used
🪛 Gitleaks (8.30.0)
defaults/main.yml
[high] 106-106: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 109-109: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 112-112: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 115-115: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 128-128: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 130-130: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 132-132: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 134-134: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 224-224: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 226-226: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 228-228: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 230-230: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~18-~18: The operating system from Apple is written “macOS”.
Context: ...tation [DataDog#676] * [IMPROVEMENT] Simplify MacOS version architecture [DataDog#677] * [IMPROVEM...
(MAC_OS)
[grammar] ~30-~30: Ensure spelling is correct
Context: ...MENT] Only gather facts and services if notd done already [DataDog#659] [Thanks @POIFischba...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~62-~62: The operating system from Apple is written “macOS”.
Context: ...eb/rpms [DataDog#624] * [IMPROVEMENT] [CI] Fix MacOS CI [DataDog#630] * [BUGFIX] Fix Ansible check ...
(MAC_OS)
[grammar] ~95-~95: Ensure spelling is correct
Context: ...ll.json file related to apm single step instrumation [DataDog#572] * [FEATURE] add support for data...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~164-~164: Use a hyphen to join words.
Context: ...ing role multiple times in a row on SUSE compatible platforms. See [DataDog#453]. * [DOC...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
4-4: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
13-13: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
21-21: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
24-24: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
27-27: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
34-34: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
38-38: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
43-43: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
49-49: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
56-56: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
60-60: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
68-68: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
78-78: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
81-81: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
85-85: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
94-94: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
105-105: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
119-119: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
123-123: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
134-134: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
137-137: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
147-147: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
152-152: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
156-156: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
168-168: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
173-173: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
178-178: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
187-187: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
192-192: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
201-201: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
208-208: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
214-214: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
226-226: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
230-230: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
234-234: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
243-243: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
247-247: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
255-255: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
263-263: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
542-542: Link and image reference definitions should be needed
Unused link or image reference definition: "DataDog#362"
(MD053, link-image-reference-definitions)
659-659: Link and image reference definitions should be needed
Unused link or image reference definition: "DataDog#664"
(MD053, link-image-reference-definitions)
667-667: Link and image reference definitions should be needed
Duplicate link or image reference definition: "DataDog#677"
(MD053, link-image-reference-definitions)
724-724: Link and image reference definitions should be needed
Unused link or image reference definition: "@POIFischbacher"
(MD053, link-image-reference-definitions)
🔇 Additional comments (4)
defaults/main.yml (4)
4-8: Well-structured addition of role enablement controls.The new top-level flags
datadog_enabledanddatadog_manage_configprovide clear control over service state and configuration management. Comments are precise and helpful.
44-44: Good expansion of macOS and multi-platform support.New variables for macOS integration (
integration_command_user_macos,datadog_agent_binary_path_macos) and agent flavor selection (datadog_agent_flavor) follow existing conventions and enable cleaner multi-platform support.Also applies to: 47-47, 65-65
82-135: Comprehensive RPM/Zypper repository and GPG key configuration.Extensive additions for yum and zypper repository management, including proxy support, GPG key rotation, and architecture-aware repo URLs. All variables are properly documented with expiration dates and checksums for transparency.
216-272: Well-organized internal variables for platform-specific defaults.Clear separation of public (lines 4–209) from internal (lines 216–272) variables. Internal macOS defaults, APT keyring paths, and architecture-specific repo URLs are properly documented as "for internal use only."
| # 5.4.1 / 2025-10-08 | ||
| * [IMPROVEMENT] Disable appstream install as its not used [#674] | ||
| * [IMPROVEMENT] Remove pycache directory during upgrades [#673] | ||
| * [IMPROVEMENT] Prepare deployment of Agent v7.70+ for macOS (arm64/x86_64) [664] |
There was a problem hiding this comment.
Fix missing "#" in pull request reference.
Line 16 lacks the "#" prefix in the PR reference, which breaks the link definition pattern used throughout the file:
-* [IMPROVEMENT] Prepare deployment of Agent v7.70+ for macOS (arm64/x86_64) [664]
+* [IMPROVEMENT] Prepare deployment of Agent v7.70+ for macOS (arm64/x86_64) [#664]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * [IMPROVEMENT] Prepare deployment of Agent v7.70+ for macOS (arm64/x86_64) [664] | |
| * [IMPROVEMENT] Prepare deployment of Agent v7.70+ for macOS (arm64/x86_64) [#664] |
🤖 Prompt for AI Agents
In CHANGELOG.md around line 16, the pull request reference is missing the "#"
prefix causing the link pattern to break; update the line to include a "#"
before the PR number so it matches the file's link reference format (e.g.,
change "[664]" to "[#664]") to restore the correct link behavior.
| [#609]: https://github.com/DataDog/ansible-datadog/pull/609 | ||
| [#611]: https://github.com/DataDog/ansible-datadog/pull/611 | ||
| [#612]: https://github.com/DataDog/ansible-datadog/pull/612 | ||
| [#618]: https://github.com/DataDog/ansible-datadog/pull/619 |
There was a problem hiding this comment.
Fix incorrect and duplicate link reference definitions.
Two critical link reference errors will cause broken navigation:
Line 635: Incorrect target PR number—should reference PR DataDog#618, not DataDog#619:
-[#618]: https://github.com/DataDog/ansible-datadog/pull/619
+[#618]: https://github.com/DataDog/ansible-datadog/pull/618Lines 666–667: Duplicate definition for [#677]—the second definition (line 667) is incorrect and should be removed:
[#677]: https://github.com/DataDog/ansible-datadog/pull/677
-[#677]: https://github.com/DataDog/ansible-datadog/pull/679Also applies to: 666-667
🤖 Prompt for AI Agents
In CHANGELOG.md around lines 635 and 666–667, fix two link-reference issues:
update the link target on line 635 to reference PR #618 (replace any occurrence
of "#619" with "#618" for that link definition), and remove the
duplicate/incorrect second definition of [#677] at line 667 (leave the first
definition intact at line 666). Ensure there are no other duplicate
link-reference definitions for these PR numbers elsewhere in the file.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tasks/agent-linux.yml (1)
98-98: Duplicated notify handler logic across multiple tasks.Lines 98, 109, 168, and 178 all contain the same complex inline Jinja2 conditional to determine which handler to notify:
{% if agent_datadog_before_7180 %}restart datadog-agent-sysprobe{% else %}restart datadog-agent{% endif %}This violates DRY (Don't Repeat Yourself) and makes future maintenance harder if the handler selection logic changes.
🔎 Recommended refactor
Extract the handler name as a fact early in the playbook:
- name: Set appropriate restart handler for system probe changes ansible.builtin.set_fact: agent_datadog_sysprobe_restart_handler: "{{ 'restart datadog-agent-sysprobe' if agent_datadog_before_7180 else 'restart datadog-agent' }}"Then simplify all notify statements:
- notify: "{% if agent_datadog_before_7180 %}restart datadog-agent-sysprobe{% else %}restart datadog-agent{% endif %}" + notify: "{{ agent_datadog_sysprobe_restart_handler }}"
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tasks/agent-linux.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (1)
tasks/agent-linux.yml (1)
117-118: Clarify the purpose of the experimental service check.Line 118 includes a condition that prevents starting
datadog-agentifdatadog-agent-exp.serviceis running. This experimental service variant is not documented in comments or elsewhere in the file.Please clarify:
- What is the
datadog-agent-exp.service?- Under what circumstances is it used?
- Should this condition be documented in a comment?
| agent_datadog_sysprobe_enabled: "{{ ((system_probe_config is defined and 'enabled' in (system_probe_config | default({}, true)) and | ||
| system_probe_config['enabled']) or (network_config is defined and 'enabled' in (network_config | default({}, true)) and | ||
| network_config['enabled']) or (service_monitoring_config is defined and | ||
| 'enabled' in (service_monitoring_config | default({}, true)) and service_monitoring_config['enabled'])) or | ||
| (system_probe_other_config is defined and (system_probe_other_config is mapping | default({}, true)) and | ||
| system_probe_other_config | length > 0) and agent_datadog_sysprobe_installed }}" |
There was a problem hiding this comment.
CRITICAL: Logic error in system probe enablement condition due to parenthesis mismatch.
Lines 81-86 have two critical issues:
-
Parenthesis mismatch: The closing
))on line 84 followed byorand thenand agent_datadog_sysprobe_installedat the end creates incorrect precedence. Currently evaluates as:((A or B or C)) or ((D and E and F) and G)This means if any of the first three conditions (system_probe_config, network_config, or service_monitoring_config) is enabled,
agent_datadog_sysprobe_enabledwill betrueeven if the service is not installed, breaking the installation check. -
Malformed expression on line 85:
(system_probe_other_config is mapping | default({}, true))attempts to apply adefaultfilter to a boolean result from theis mappingtest, which is invalid Jinja2 syntax.
🔎 Proposed fix
- name: Set system probe enabled (since 6/7.61.0)
ansible.builtin.set_fact:
agent_datadog_sysprobe_enabled: "{{ ((system_probe_config is defined and 'enabled' in (system_probe_config | default({}, true)) and
system_probe_config['enabled']) or (network_config is defined and 'enabled' in (network_config | default({}, true)) and
network_config['enabled']) or (service_monitoring_config is defined and
- 'enabled' in (service_monitoring_config | default({}, true)) and service_monitoring_config['enabled'])) or
- (system_probe_other_config is defined and (system_probe_other_config is mapping | default({}, true)) and
- system_probe_other_config | length > 0) and agent_datadog_sysprobe_installed }}"
+ 'enabled' in (service_monitoring_config | default({}, true)) and service_monitoring_config['enabled']) or
+ (system_probe_other_config is defined and system_probe_other_config is mapping and
+ system_probe_other_config | length > 0)) and agent_datadog_sysprobe_installed }}"
when: not datadog_skip_running_check and (not agent_datadog_before_7610)![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
28 issues found across 91 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".circleci/config.yml">
<violation number="1" location=".circleci/config.yml:269">
P0: Invalid bash array syntax. `["java", "js", ...]` is not valid bash - it will be interpreted as a glob pattern, not iterate over the list. Use `for tracer in java js dotnet python ruby; do` instead.</violation>
<violation number="2" location=".circleci/config.yml:316">
P3: Typo: "Pythpn" should be "Python".</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:646">
P2: URL uses `docs.datadog.com` instead of `docs.datadoghq.com`. This is inconsistent with all other Datadog documentation links in this file and may result in a broken link.</violation>
</file>
<file name="tasks/integration/linux.yml">
<violation number="1" location="tasks/integration/linux.yml:6">
P3: Typo in task name: "commmand" should be "command" (has 3 m's instead of 2).</violation>
</file>
<file name="tasks/integration/macos.yml">
<violation number="1" location="tasks/integration/macos.yml:6">
P3: Typo in task name: "commmand" should be "command" (has three 'm's instead of two).</violation>
</file>
<file name="tasks/windows_remote_update_check.yml">
<violation number="1" location="tasks/windows_remote_update_check.yml:6">
P2: Add `changed_when: false` since this shell task only reads process information and doesn't modify the system. Without this, Ansible will always report this task as "changed" even on subsequent runs.</violation>
</file>
<file name="tasks/win_agent_latest.yml">
<violation number="1" location="tasks/win_agent_latest.yml:10">
P1: Trailing space introduced in URL. There's a space after `{{ datadog_windows_agent7_latest_url }}` before `{% else %}` which will be included in the resulting URL when major version is 7, potentially causing download failures.</violation>
</file>
<file name="CONTRIBUTING.md">
<violation number="1" location="CONTRIBUTING.md:55">
P2: Referenced script `ci_test/collection-structure-setup.sh` does not exist. The `ci_test/` directory contains `ansible_lint.sh` instead. This will cause contributors to fail when following the documentation.</violation>
</file>
<file name="manual_tests/Vagrantfile">
<violation number="1" location="manual_tests/Vagrantfile:3">
P2: Ubuntu 14.04 Trusty reached End of Life in April 2019. For an "Annual Update," consider using a currently supported Ubuntu LTS version like `ubuntu/jammy64` (22.04) or `ubuntu/focal64` (20.04).</violation>
<violation number="2" location="manual_tests/Vagrantfile:7">
P2: CentOS 8 reached End of Life in December 2021. Consider using `rockylinux/9` or `almalinux/9` as actively maintained RHEL-compatible alternatives.</violation>
</file>
<file name=".github/dependabot.yml">
<violation number="1" location=".github/dependabot.yml:3">
P2: Incorrect package ecosystem: `bundler` is for Ruby projects, but this repository has no Gemfile. Consider using `pip` for the Python requirements.txt files, or `github-actions` for workflow dependencies instead.</violation>
</file>
<file name="ci_test/install_agent_7_macos.yaml">
<violation number="1" location="ci_test/install_agent_7_macos.yaml:10">
P3: Misleading comment: This file is specifically for macOS (`install_agent_7_macos.yaml`), but the comment references CentOS. Consider updating the comment to reflect the actual reason for skipping the running check on macOS, or remove it if not applicable.</violation>
</file>
<file name="tasks/pkg-suse/install-pinned.yml">
<violation number="1" location="tasks/pkg-suse/install-pinned.yml:3">
P1: The `oldpackage` parameter is not supported by `ansible.builtin.package` - it's a zypper-specific option. This task will fail or the downgrade logic won't work. Consider using `ansible.builtin.zypper` instead to retain the `oldpackage` functionality.</violation>
</file>
<file name="tasks/parse-version-macos.yml">
<violation number="1" location="tasks/parse-version-macos.yml:4">
P2: Shell variable should use the `quote` filter for paths that may contain spaces or special characters. macOS paths commonly contain spaces, and without proper quoting the command will fail.</violation>
</file>
<file name="CHANGELOG.md">
<violation number="1" location="CHANGELOG.md:16">
P3: Missing `#` in link reference: `[664]` should be `[#664]` for consistent markdown link formatting.</violation>
<violation number="2" location="CHANGELOG.md:31">
P3: Typo: `[IMROVEMENT]` should be `[IMPROVEMENT]`.</violation>
<violation number="3" location="CHANGELOG.md:635">
P2: Incorrect URL: Link reference `[#618]` points to `/pull/619` instead of `/pull/618`. The reference number should match the pull request number.</violation>
<violation number="4" location="CHANGELOG.md:667">
P2: Duplicate link reference: `[#677]` is defined twice. This second definition should be `[#679]` to match its URL.</violation>
</file>
<file name="tasks/pkg-windows-opts.yml">
<violation number="1" location="tasks/pkg-windows-opts.yml:126">
P1: Version comparison logic is incorrect for major versions less than 7. For example, version 6.50 would fail the check `agent_datadog_minor | int < 45` (50 < 45 is false), even though 6.50 is definitely before 7.45. The condition should be `(agent_datadog_major | int < 7) or (agent_datadog_major | int == 7 and agent_datadog_minor | int < 45)`.</violation>
</file>
<file name="tasks/agent-linux.yml">
<violation number="1" location="tasks/agent-linux.yml:14">
P1: Inconsistent variable names used in version check. This task uses `datadog_major`/`datadog_minor`/`datadog_bugfix` while all other similar flag tasks use `agent_datadog_major`/`agent_datadog_minor`/`agent_datadog_bugfix`. This will likely cause the check to always be undefined/false.</violation>
<violation number="2" location="tasks/agent-linux.yml:85">
P1: Invalid Jinja2 syntax: `is mapping | default({}, true)` is incorrect. The `is mapping` test returns a boolean which cannot be piped to `default()`. Should be `(system_probe_other_config | default({}, true)) is mapping` to first apply the default and then test if it's a mapping.</violation>
<violation number="3" location="tasks/agent-linux.yml:85">
P1: Operator precedence bug: `agent_datadog_sysprobe_installed` only applies to the last OR clause due to missing parentheses. The system probe could be marked as enabled even when not installed if `system_probe_config`, `network_config`, or `service_monitoring_config` are enabled. The entire expression should be wrapped with `and agent_datadog_sysprobe_installed` at the end.</violation>
</file>
<file name="tasks/pkg-debian.yml">
<violation number="1" location="tasks/pkg-debian.yml:48">
P1: Invalid YAML multi-line string syntax for task name. Without using `>` or `|` operators, the second line is not properly parsed as part of the `name` value, which may cause YAML parsing errors or unexpected behavior. Consider using folded style or keeping the name on a single line.</violation>
</file>
<file name="tasks/main.yml">
<violation number="1" location="tasks/main.yml:8">
P1: The `when` conditions use AND logic (list format) but this will error if `ansible_facts` is undefined because `ansible_facts.system` cannot be accessed on an undefined variable. This should use OR logic instead.</violation>
<violation number="2" location="tasks/main.yml:27">
P3: Typo in task name: 'presidence' should be 'precedence'.</violation>
</file>
<file name=".ansible-lint">
<violation number="1" location=".ansible-lint:8">
P2: Inconsistent rule naming: `var_naming` uses underscore but ansible-lint canonical rule names use hyphens. This should be `var-naming` to match the format used in `skip_list` and ensure the warning is properly configured.</violation>
</file>
<file name="tasks/pkg-macos/macos_agent_latest.yml">
<violation number="1" location="tasks/pkg-macos/macos_agent_latest.yml:16">
P2: `retries: 2` has no effect without an `until` condition. In Ansible, retries only work when combined with `until`. Without it, the task runs exactly once regardless of the `retries` value. Consider adding `until` and `delay` parameters, e.g.:
```yaml
retries: 2
delay: 3
until: datadog_macos_agent7_latest_arch_check.status is defined
```</violation>
</file>
<file name="tasks/agent-win.yml">
<violation number="1" location="tasks/agent-win.yml:137">
P1: The `retries` and `delay` parameters have no effect without an `until` condition. This task will only run once, not retry 3 times as intended. Add an `until` condition and `register` to enable actual retry behavior.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
| os: ["rocky9", "debian"] | ||
| python: ["python3.11"] | ||
|
|
||
| # Newer debian images only have Pythpn 3 installed |
There was a problem hiding this comment.
P3: Typo: "Pythpn" should be "Python".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .circleci/config.yml, line 316:
<comment>Typo: "Pythpn" should be "Python".</comment>
<file context>
@@ -90,28 +138,269 @@ jobs:
+ os: ["rocky9", "debian"]
+ python: ["python3.11"]
+
+ # Newer debian images only have Pythpn 3 installed
+ - test_install_downgrade:
+ matrix:
</file context>
| # Newer debian images only have Pythpn 3 installed | |
| # Newer debian images only have Python 3 installed |
| - run: sudo datadog-agent status || true | ||
| - run: ps aux | grep -v grep | grep datadog-agent | ||
| - run: > | ||
| bash -c 'for tracer in ["java", "js", "dotnet", "python", "ruby"]; do |
There was a problem hiding this comment.
P0: Invalid bash array syntax. ["java", "js", ...] is not valid bash - it will be interpreted as a glob pattern, not iterate over the list. Use for tracer in java js dotnet python ruby; do instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .circleci/config.yml, line 269:
<comment>Invalid bash array syntax. `["java", "js", ...]` is not valid bash - it will be interpreted as a glob pattern, not iterate over the list. Use `for tracer in java js dotnet python ruby; do` instead.</comment>
<file context>
@@ -90,28 +138,269 @@ jobs:
+ - run: sudo datadog-agent status || true
+ - run: ps aux | grep -v grep | grep datadog-agent
+ - run: >
+ bash -c 'for tracer in ["java", "js", "dotnet", "python", "ruby"]; do
+ sudo datadog-installer is-installed datadog-apm-library-$tracer;
+ done'
</file context>
| bash -c 'for tracer in ["java", "js", "dotnet", "python", "ruby"]; do | |
| bash -c 'for tracer in java js dotnet python ruby; do |
| [18]: https://docs.datadoghq.com/security/cspm/setup/?tab=docker | ||
| [19]: https://github.com/DataDog/integrations-core | ||
| [20]: https://github.com/DataDog/integrations-extras | ||
| [21]: https://docs.datadog.com/tracing/trace_collection/library_injection_local |
There was a problem hiding this comment.
P2: URL uses docs.datadog.com instead of docs.datadoghq.com. This is inconsistent with all other Datadog documentation links in this file and may result in a broken link.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 646:
<comment>URL uses `docs.datadog.com` instead of `docs.datadoghq.com`. This is inconsistent with all other Datadog documentation links in this file and may result in a broken link.</comment>
<file context>
@@ -505,15 +600,47 @@ If you are updating from **6.14.0 or 6.14.1 on Windows**, use the following step
+[18]: https://docs.datadoghq.com/security/cspm/setup/?tab=docker
+[19]: https://github.com/DataDog/integrations-core
+[20]: https://github.com/DataDog/integrations-extras
+[21]: https://docs.datadog.com/tracing/trace_collection/library_injection_local
</file context>
| [21]: https://docs.datadog.com/tracing/trace_collection/library_injection_local | |
| [21]: https://docs.datadoghq.com/tracing/trace_collection/library_injection_local |
| ansible.builtin.set_fact: | ||
| datadog_agent_binary_path: "{{ datadog_agent_binary_path_linux }}" | ||
|
|
||
| - name: Set Agent user for integration commmand |
There was a problem hiding this comment.
P3: Typo in task name: "commmand" should be "command" (has 3 m's instead of 2).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tasks/integration/linux.yml, line 6:
<comment>Typo in task name: "commmand" should be "command" (has 3 m's instead of 2).</comment>
<file context>
@@ -0,0 +1,12 @@
+ ansible.builtin.set_fact:
+ datadog_agent_binary_path: "{{ datadog_agent_binary_path_linux }}"
+
+- name: Set Agent user for integration commmand
+ ansible.builtin.set_fact:
+ integration_command_user: "{{ integration_command_user_linux }}"
</file context>
| ansible.builtin.set_fact: | ||
| datadog_agent_binary_path: "{{ datadog_agent_binary_path_macos }}" | ||
|
|
||
| - name: Set Agent user for integration commmand |
There was a problem hiding this comment.
P3: Typo in task name: "commmand" should be "command" (has three 'm's instead of two).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tasks/integration/macos.yml, line 6:
<comment>Typo in task name: "commmand" should be "command" (has three 'm's instead of two).</comment>
<file context>
@@ -0,0 +1,12 @@
+ ansible.builtin.set_fact:
+ datadog_agent_binary_path: "{{ datadog_agent_binary_path_macos }}"
+
+- name: Set Agent user for integration commmand
+ ansible.builtin.set_fact:
+ integration_command_user: "{{ integration_command_user_macos }}"
</file context>
| msg: "datadog_api_key is mandatory when using managed config" | ||
| when: datadog_api_key is not defined and datadog_manage_config | ||
|
|
||
| - name: Resolve datadog_tracked_checks later to defend against variable presidence issues arising from dynamically included null datadog_checks |
There was a problem hiding this comment.
P3: Typo in task name: 'presidence' should be 'precedence'.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tasks/main.yml, line 27:
<comment>Typo in task name: 'presidence' should be 'precedence'.</comment>
<file context>
@@ -1,47 +1,128 @@
+ msg: "datadog_api_key is mandatory when using managed config"
+ when: datadog_api_key is not defined and datadog_manage_config
+- name: Resolve datadog_tracked_checks later to defend against variable presidence issues arising from dynamically included null datadog_checks
+ ansible.builtin.include_tasks: sanitize-checks.yml
+
</file context>
| - name: Resolve datadog_tracked_checks later to defend against variable presidence issues arising from dynamically included null datadog_checks | |
| - name: Resolve datadog_tracked_checks later to defend against variable precedence issues arising from dynamically included null datadog_checks |
| # If the full prefix isn't specified in Ansible 2.10+, we might end up running `ansible.windows.setup` instead. | ||
| when: | ||
| - ansible_facts is undefined | ||
| - ansible_facts.system is undefined |
There was a problem hiding this comment.
P1: The when conditions use AND logic (list format) but this will error if ansible_facts is undefined because ansible_facts.system cannot be accessed on an undefined variable. This should use OR logic instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tasks/main.yml, line 8:
<comment>The `when` conditions use AND logic (list format) but this will error if `ansible_facts` is undefined because `ansible_facts.system` cannot be accessed on an undefined variable. This should use OR logic instead.</comment>
<file context>
@@ -1,47 +1,128 @@
+ # If the full prefix isn't specified in Ansible 2.10+, we might end up running `ansible.windows.setup` instead.
+ when:
+ - ansible_facts is undefined
+ - ansible_facts.system is undefined
+
+- name: Initialize internal datadog_config variable
</file context>
| - fqcn[action] | ||
|
|
||
| warn_list: | ||
| - var_naming |
There was a problem hiding this comment.
P2: Inconsistent rule naming: var_naming uses underscore but ansible-lint canonical rule names use hyphens. This should be var-naming to match the format used in skip_list and ensure the warning is properly configured.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .ansible-lint, line 8:
<comment>Inconsistent rule naming: `var_naming` uses underscore but ansible-lint canonical rule names use hyphens. This should be `var-naming` to match the format used in `skip_list` and ensure the warning is properly configured.</comment>
<file context>
@@ -0,0 +1,12 @@
+ - fqcn[action]
+
+warn_list:
+ - var_naming
+ - idiom
+
</file context>
| status_code: 200 | ||
| register: datadog_macos_agent7_latest_arch_check | ||
| failed_when: false | ||
| retries: 2 |
There was a problem hiding this comment.
P2: retries: 2 has no effect without an until condition. In Ansible, retries only work when combined with until. Without it, the task runs exactly once regardless of the retries value. Consider adding until and delay parameters, e.g.:
retries: 2
delay: 3
until: datadog_macos_agent7_latest_arch_check.status is definedPrompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tasks/pkg-macos/macos_agent_latest.yml, line 16:
<comment>`retries: 2` has no effect without an `until` condition. In Ansible, retries only work when combined with `until`. Without it, the task runs exactly once regardless of the `retries` value. Consider adding `until` and `delay` parameters, e.g.:
```yaml
retries: 2
delay: 3
until: datadog_macos_agent7_latest_arch_check.status is defined
```</comment>
<file context>
@@ -0,0 +1,30 @@
+ status_code: 200
+ register: datadog_macos_agent7_latest_arch_check
+ failed_when: false
+ retries: 2
+ when:
+ - datadog_macos_download_url | default('', true) | length == 0
</file context>
| - (datadog_start_result.failed | default(false)) | ||
| - not datadog_remote_update_in_progress | ||
| delay: 10 | ||
| retries: 3 |
There was a problem hiding this comment.
P1: The retries and delay parameters have no effect without an until condition. This task will only run once, not retry 3 times as intended. Add an until condition and register to enable actual retry behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tasks/agent-win.yml, line 137:
<comment>The `retries` and `delay` parameters have no effect without an `until` condition. This task will only run once, not retry 3 times as intended. Add an `until` condition and `register` to enable actual retry behavior.</comment>
<file context>
@@ -1,48 +1,154 @@
+ - (datadog_start_result.failed | default(false))
+ - not datadog_remote_update_in_progress
+ delay: 10
+ retries: 3
+
+- name: Ensure datadog-agent is disabled
</file context>
| retries: 3 | |
| retries: 3 | |
| until: datadog_retry_result.state == 'started' | |
| register: datadog_retry_result |
Update CHANGELOG.md Fix link to PR related to 5.4.0 Co-authored-by: ian.bucad <ian.bucad@datadoghq.com>
* update datadog_agent_binary_path_linux to new path * update macos ci runner to xcode 26.2.0 * detect which path to use instead * fix tests * allow integration install to fail, just checking binary path * fix binary path detection logic * clean up tests * datadog_agent_binary_path falls back to datadog_agent_binary_path_linux
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.circleci/config.yml:
- Line 342: Fix the typo "Pythpn" to "Python" in the comments that mention
Python in the CI config; locate the comment lines containing the exact
misspelling "Pythpn" (both occurrences referenced in the diff) and replace them
with "Python" so the documentation comments read correctly.
- Around line 268-271: The bash for-loop uses JSON-style array syntax which is
invalid; edit the run command so the loop header uses a bash space-separated
list (java js dotnet python ruby) instead of ["java", "js", "dotnet", "python",
"ruby"] and ensure the loop uses the proper bash structure (for tracer in ...;
do ...; done) while leaving the body invocation sudo datadog-installer
is-installed datadog-apm-library-$tracer unchanged.
In `@ci_test/install_agent_7_with_integration_fleet.yaml`:
- Around line 7-15: Add the missing Datadog control vars to the playbook's vars
block: set datadog_enabled: false and datadog_skip_running_check: true so the
agent does not start during CI; update the vars alongside
datadog_agent_major_version, datadog_api_key,
datadog_apm_instrumentation_enabled, datadog_apm_instrumentation_libraries and
datadog_integration (e.g., next to datadog_agent_major_version) to match the
other test playbooks.
🧹 Nitpick comments (1)
ci_test/install_agent_7_with_integration_fleet.yaml (1)
24-24: Missing trailing newline.YAML files should end with a newline character for POSIX compliance and to avoid potential issues with some tools.
Add trailing newline
always: - name: Verify Fleet path was detected ansible.builtin.assert: that: datadog_agent_binary_path == "/bin/datadog-agent" fail_msg: "Wrong path: {{ datadog_agent_binary_path }}" +
| - run: > | ||
| bash -c 'for tracer in ["java", "js", "dotnet", "python", "ruby"]; do | ||
| sudo datadog-installer is-installed datadog-apm-library-$tracer; | ||
| done' |
There was a problem hiding this comment.
Invalid bash for-loop syntax will cause job failure.
The loop syntax for tracer in ["java", "js", "dotnet", "python", "ruby"] is incorrect. Bash does not use JSON-style array notation for iteration.
🐛 Proposed fix
- run: >
- bash -c 'for tracer in ["java", "js", "dotnet", "python", "ruby"]; do
+ bash -c 'for tracer in java js dotnet python ruby; do
sudo datadog-installer is-installed datadog-apm-library-$tracer;
done'🤖 Prompt for AI Agents
In @.circleci/config.yml around lines 268 - 271, The bash for-loop uses
JSON-style array syntax which is invalid; edit the run command so the loop
header uses a bash space-separated list (java js dotnet python ruby) instead of
["java", "js", "dotnet", "python", "ruby"] and ensure the loop uses the proper
bash structure (for tracer in ...; do ...; done) while leaving the body
invocation sudo datadog-installer is-installed datadog-apm-library-$tracer
unchanged.
| os: ["rocky9", "debian"] | ||
| python: ["python3.11"] | ||
|
|
||
| # Newer debian images only have Pythpn 3 installed |
There was a problem hiding this comment.
Typo: "Pythpn" should be "Python".
Lines 342 and 351 both contain the typo "Pythpn" instead of "Python" in the comments.
📝 Proposed fix
- # Newer debian images only have Pythpn 3 installed
+ # Newer debian images only have Python 3 installed📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Newer debian images only have Pythpn 3 installed | |
| # Newer debian images only have Python 3 installed |
🤖 Prompt for AI Agents
In @.circleci/config.yml at line 342, Fix the typo "Pythpn" to "Python" in the
comments that mention Python in the CI config; locate the comment lines
containing the exact misspelling "Pythpn" (both occurrences referenced in the
diff) and replace them with "Python" so the documentation comments read
correctly.
| vars: | ||
| datadog_agent_major_version: 7 | ||
| datadog_api_key: "11111111111111111111111111111111" | ||
| datadog_apm_instrumentation_enabled: "host" | ||
| datadog_apm_instrumentation_libraries: ["python"] | ||
| datadog_integration: | ||
| test-integration: | ||
| action: install | ||
| version: 1.0.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check other test files for these settings patterns
echo "=== All test files in ci_test directory ==="
ls -la ci_test/*.yaml 2>/dev/null | head -20
echo -e "\n=== Checking for datadog_enabled and datadog_skip_running_check in all test files ==="
rg -n 'datadog_enabled|datadog_skip_running_check' ci_test/ --color=never
echo -e "\n=== Checking Fleet-related test files specifically ==="
rg -l 'fleet|Fleet' ci_test/ --color=never
echo -e "\n=== Content of install_agent_7_with_integration.yaml for comparison ==="
cat ci_test/install_agent_7_with_integration.yaml
echo -e "\n=== Checking trailing newline in the Fleet file ==="
tail -c 20 ci_test/install_agent_7_with_integration_fleet.yaml | od -cRepository: NickJLange/ansible-datadog
Length of output: 3205
🏁 Script executed:
#!/bin/bash
# Verify trailing newline more reliably
echo "=== Checking trailing newline status ==="
echo "Fleet file ends with newline:"
tail -1 ci_test/install_agent_7_with_integration_fleet.yaml | wc -l
echo -e "\nComparison file (install_agent_7_with_integration.yaml) ends with newline:"
tail -1 ci_test/install_agent_7_with_integration.yaml | wc -l
echo -e "\n=== File sizes and last few bytes ==="
echo "Fleet file:"
wc -l ci_test/install_agent_7_with_integration_fleet.yaml
tail -c 30 ci_test/install_agent_7_with_integration_fleet.yaml | xxd
echo -e "\nComparison file:"
wc -l ci_test/install_agent_7_with_integration.yaml
tail -c 30 ci_test/install_agent_7_with_integration.yaml | xxd
echo -e "\n=== Full content of Fleet file ==="
cat -n ci_test/install_agent_7_with_integration_fleet.yamlRepository: NickJLange/ansible-datadog
Length of output: 1601
Add datadog_enabled: false and datadog_skip_running_check: true for consistency.
This playbook is the only test file missing these settings, which are present in all other test files (install_agent_7.yaml, install_agent_7_with_integration.yaml, install_agent_7_pinned.yaml, etc.). Without them, the Datadog agent service may start during CI tests. Add these settings to the vars block to match the established test pattern.
🤖 Prompt for AI Agents
In `@ci_test/install_agent_7_with_integration_fleet.yaml` around lines 7 - 15, Add
the missing Datadog control vars to the playbook's vars block: set
datadog_enabled: false and datadog_skip_running_check: true so the agent does
not start during CI; update the vars alongside datadog_agent_major_version,
datadog_api_key, datadog_apm_instrumentation_enabled,
datadog_apm_instrumentation_libraries and datadog_integration (e.g., next to
datadog_agent_major_version) to match the other test playbooks.
…it can be executed (#694)
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name=".circleci/config.yml">
<violation number="1" location=".circleci/config.yml:316">
P2: The `run: >` folded block collapses newlines, so the first `#` comment causes the rest of the `bash -c` script to be commented out and skipped. Use a literal block (`|`) or remove comments/insert semicolons so the checks actually execute.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| - run: sudo datadog-agent status || true | ||
| - run: ps aux | grep -v grep | grep datadog-agent | ||
| # Verify that SSI was installed with custom directory | ||
| - run: > |
There was a problem hiding this comment.
P2: The run: > folded block collapses newlines, so the first # comment causes the rest of the bash -c script to be commented out and skipped. Use a literal block (|) or remove comments/insert semicolons so the checks actually execute.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .circleci/config.yml, line 316:
<comment>The `run: >` folded block collapses newlines, so the first `#` comment causes the rest of the `bash -c` script to be commented out and skipped. Use a literal block (`|`) or remove comments/insert semicolons so the checks actually execute.</comment>
<file context>
@@ -296,6 +296,45 @@ jobs:
+ - run: sudo datadog-agent status || true
+ - run: ps aux | grep -v grep | grep datadog-agent
+ # Verify that SSI was installed with custom directory
+ - run: >
+ bash -c '
+ # Check that datadog-installer is present
</file context>
| - run: > | |
| - run: | |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="defaults/main.yml">
<violation number="1" location="defaults/main.yml:208">
P1: Defaulting the SSI script directory to /tmp introduces a local privilege escalation risk: an unprivileged user can pre-create /tmp/datadog-installer and keep ownership since the role doesn’t set owner/group, then swap the installer script before it runs. Use a root-owned runtime directory by default (or set owner/group explicitly in the tasks).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| # Directory where the Single Step Instrumentation (SSI) script will be downloaded and executed. | ||
| # Default uses /tmp/datadog-installer. | ||
| # Override this if /tmp/datadog-installer is not suitable in your environment. | ||
| datadog_ssi_script_dir: "/tmp/datadog-installer" |
There was a problem hiding this comment.
P1: Defaulting the SSI script directory to /tmp introduces a local privilege escalation risk: an unprivileged user can pre-create /tmp/datadog-installer and keep ownership since the role doesn’t set owner/group, then swap the installer script before it runs. Use a root-owned runtime directory by default (or set owner/group explicitly in the tasks).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At defaults/main.yml, line 208:
<comment>Defaulting the SSI script directory to /tmp introduces a local privilege escalation risk: an unprivileged user can pre-create /tmp/datadog-installer and keep ownership since the role doesn’t set owner/group, then swap the installer script before it runs. Use a root-owned runtime directory by default (or set owner/group explicitly in the tasks).</comment>
<file context>
@@ -203,9 +203,9 @@ datadog_apm_inject_version: ""
-datadog_ssi_script_dir: "/var/run/datadog-ssi"
+# Default uses /tmp/datadog-installer.
+# Override this if /tmp/datadog-installer is not suitable in your environment.
+datadog_ssi_script_dir: "/tmp/datadog-installer"
# Enable remote updates through datadog-installer
</file context>
| datadog_ssi_script_dir: "/tmp/datadog-installer" | |
| datadog_ssi_script_dir: "/var/run/datadog-installer" |
|
Closing stale PR on inactive fork |
Catching up on upstream
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores