Skip to content

[Windows] Force reinstall if configuration changed [AP-1946] #509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 24, 2023

Conversation

chouquette
Copy link
Contributor

@chouquette chouquette commented Jul 24, 2023

This PR adds support for optional features exposed by the MSI installer.
It will probe the available features and will trigger a reinstall if an optional features must be enabled/disabled (the installer doesn't support in place modification of the installed features)

While this is of limited interest given that the installer always installs all features since 7.46, this paves the way for future optional features that we might not want to systematically install

@chouquette chouquette requested a review from a team as a code owner July 24, 2023 14:02
@chouquette chouquette added the bug label Jul 24, 2023
@chouquette chouquette changed the title [Windows] Force reinstall if configuration changed [Windows] Force reinstall if configuration changed [AP-1946] Jul 24, 2023
set_fact:
agent_datadog_skip_install: >-
{{ agent_config_install_args.exists and
agent_config_install_args.value != agent_win_install_args }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we skip the install when the provided args match the previous one stored in the registry? It looks like agent_config_install_args.value != agent_win_install_args will set agent_datadog_skip_install to true when they are not the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, indeed you are correct the logic is inverted, which means something else is wrong since the install was still forced during my tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the issue, it was somewhere between the chair and the keyboard (I forgot to pin the version so I was checking with a agent_datadog_skip_install set to false, meaning I didn't test anything valuable)

@chouquette chouquette marked this pull request as draft July 25, 2023 08:07
@chouquette chouquette force-pushed the chouquette/detect_win_config_change branch from bca37b8 to 14dcaf8 Compare July 27, 2023 08:31
@chouquette chouquette marked this pull request as ready for review July 27, 2023 08:32
@chouquette chouquette requested a review from a team July 27, 2023 08:35
@chouquette chouquette force-pushed the chouquette/detect_win_config_change branch from 73043a3 to 97e8015 Compare August 18, 2023 14:20
@@ -12,6 +16,23 @@
agent_win_install_args: "{{ win_install_args }} DDAGENTUSER_NAME={{ datadog_windows_ddagentuser_name }}"
when: datadog_windows_ddagentuser_name | default('', true) | length > 0

- name: Get Windows Agent config
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Get Windows Agent config
- name: Get Windows Agent features

The term config for the Agent normally refers to the configuration files, I think this could cause confusion here since this code extracts the installer features.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed

@@ -90,3 +111,16 @@
set_fact:
agent_win_install_args: "{{ agent_win_install_args }} ADDLOCAL=MainApplication,NPM"
when: agent_datadog_sysprobe_enabled

# Check for potential config changes that require a reinstall:
- name: Check NPM config change
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: Check NPM config change
- name: Check NPM feature change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed

# reported as such regardless of the provided parameters. Since we don't
# want to force the install for nothing in those version, we skip the step
# when the agent is 7.45+
when: >-
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I wonder if we should also print a message to inform the user of this change in behavior ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a debug message to let the users know

This will help for [AP-1946] and allows us to detect if the NPM driver
is installed on old agent installations
We will soon override agent_datadog_skip_install within this file so the
updated value must be available ASAP
@chouquette chouquette force-pushed the chouquette/detect_win_config_change branch from 97e8015 to f463387 Compare October 3, 2023 15:13
@chouquette
Copy link
Contributor Author

chouquette commented Oct 3, 2023

Thanks for the review! I pushed an additional commit per your suggestions (and rebased on top of main)

Copy link
Member

@KevinFairise2 KevinFairise2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@chouquette chouquette merged commit badb008 into main Oct 24, 2023
@chouquette chouquette deleted the chouquette/detect_win_config_change branch October 24, 2023 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants