Skip to content

Conversation

@KSerrania
Copy link
Contributor

@KSerrania KSerrania commented Nov 5, 2019

What does this PR do?

Adds Agent 7 install support.

The datadog_agent5 option is deprecated and replaced by datadog_agent_major_version, which can be set to 5, 6 or 7.
If set, the datadog_agent_version option must have the major version specified in datadog_agent_major_version.

For Agent 6 and 7, the datadog_apt_repo, datadog_yum_repo, datadog_zypper_repo options have been changed: if they're not set, the module will use the official Datadog repository
for the major version specified in datadog_agent_major_version. If they're set, the specified repository will be used, regardless of the datadog_agent_major_version set.

WIP

  • Test Agent 7 install (tested with 7.15.0~beta.1), and various upgrade / downgrade scenarios.
  • Manual test on SUSE
  • Manual test on Windows
  • Add version changes to README + upgrade notes

@KSerrania KSerrania force-pushed the kserrania/agent7-support branch 10 times, most recently from 876dae6 to d494f7f Compare November 6, 2019 12:59
@KSerrania KSerrania marked this pull request as ready for review November 6, 2019 13:39
@KSerrania KSerrania force-pushed the kserrania/agent7-support branch from d494f7f to 22daa4d Compare November 6, 2019 13:42
@KSerrania KSerrania changed the title [WIP] Agent 7 support Agent 7 support Nov 6, 2019
@albertvaka
Copy link
Contributor

README needs to be updated

@albertvaka albertvaka requested a review from hush-hush November 20, 2019 11:01
KSerrania and others added 5 commits November 25, 2019 10:51
If datadog_agent_version is set, extracts the major version from it and sets datadog_agent_major_version with this value.
If datadog_agent_major_version is already set, checks that the two variables are compatible, otherwise raises an error.

Co-Authored-By: Albert Vaca <albert.vaca@datadoghq.com>
Copy link
Member

@hush-hush hush-hush left a comment

Choose a reason for hiding this comment

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

I added a small comment. Feel free to merge after addressing it !

README.md Outdated
vars:
datadog_api_key: "123456"
datadog_agent_version: "1:6.13.0-1" # for apt-based platforms, use a `6.13.0-1` format on yum-based platforms and `6.13.0` for Windows
datadog_agent_version: "1:7.16.0-1" # apt-based format; the role will deduce and use the `7.16.0-1` format on yum-based platforms and `7.16.0` on Windows
Copy link
Member

Choose a reason for hiding this comment

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

We should just use the 7.16.0 format. The rest is only here to be backward compatible.

README.md Outdated

Install and configure Datadog Agent & checks. Starting with version `2` of this role Datadog Agent version 6 is installed by default (instead of version 5).
Install and configure Datadog Agent & checks.
The version `3` of this role installs the Datadog Agent version 7 by default.
Copy link
Member

Choose a reason for hiding this comment

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

Just realize that it's version 4 ! the role is already in version 3.3.0

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 just realized too! I'm going to change v3 -> v4 and v2 -> v3.

@KSerrania KSerrania requested a review from hush-hush December 17, 2019 19:44
@rromanchuk
Copy link

@KSerrania i'm going to give your fork a go, thanks for this

Copy link
Member

@olivielpeau olivielpeau left a comment

Choose a reason for hiding this comment

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

Left a question inline, and a few comments, LGTM overall.

One more suggestion: In defaults/main.yml, I think we should more clearly separate the internal variables that are not part of the stable API of the role, from the variables that are part of the stable API. Typically, put all the internal variables at the end of the file in a section clearly separated with a very visible comment.

- name: Remove previous datadog apt list file
file:
path: /etc/apt/sources.list.d/ansible_datadog_agent.list
state: absent
Copy link
Member

Choose a reason for hiding this comment

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

not sure I understand what this task is for: if I understand correctly the tasks below will update the /etc/apt/sources.list.d/ansible_datadog_agent.list anyway, so why do we need to delete it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The custom repository won't get deleted if this is not present. Moreover, if the internal variables (datadog_agentX_yyyyy_repo) are modified, the previous values won't get removed from the source files, which may cause issues. IMO it's safer to explicitly remove the list file.

It does have the downside of making the role not idempotent though, so this is up for discussion.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, thanks for the explanation, makes sense. Indeed, the idempotency would be nice to have, but not blocking for this PR.

In a future version, we may want to revisit using the apt_repository task. For our use case, I think all of this logic would be easier to implement with one single file task that would trigger apt-get update on the datadog repo whenever needed. Not necessary for now though.

KSerrania and others added 2 commits December 18, 2019 14:31
Co-Authored-By: Olivier Vielpeau <olivielpeau@users.noreply.github.com>
@KSerrania KSerrania merged commit 3787f2e into master Dec 18, 2019
@KSerrania KSerrania deleted the kserrania/agent7-support branch December 18, 2019 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants