-
Notifications
You must be signed in to change notification settings - Fork 261
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
Add manage_install option #608
Conversation
Service code is the same for Ubuntu and Redhat
Since we use it later on as 'require'.
9a168f3
to
9f824ed
Compare
9f824ed
to
c5393fd
Compare
11d900b
to
79762cf
Compare
79762cf
to
3f9b93b
Compare
manifests/init.pp
Outdated
@@ -67,8 +67,10 @@ | |||
# Set the value of the statsd_forward_port varable. Used to forward all | |||
# statsd metrics to another host. | |||
# $manage_repo | |||
# Boolean to indicate whether this module should attempt to manage | |||
# the package repo. Only for RPM-based distros. Default true. | |||
# Deprecated. Use $manage_install instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this really be deprecated? I think $manage_repo
and $manage_install
have two different purposes:
$manage_install
indicates that you want the module to install the Agent (which you can turn off if you have custom modules installing it for you)$manage_repo
indicates that you want the module to set up the Agent repo (which you can turn off if you have custom modules setting it up for you).
Moreover, the current PR does not really fix #604, as it's still not possible to install the Agent without setting up the repos on Ubuntu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true that manage_install
has more implications than manage_repo
but since manage_repo
wasn't implemented for Ubuntu, I thought that a wider thing like manage_install
(which we needed anyway for #476) could also work for both use cases.
Maybe the wording of the deprecation message should be Set manage_install to false and install datadog-agent manually
to achieve the same thing as before. I'll change this.
Another option would be to keep both manage_repo
and manage_install
but then we have to implement it for Ubuntu as well and given how little these options are used, I prefer having one flag that two. Too many options increase complexity.
# | ||
|
||
class datadog_agent::service( | ||
$service_ensure = 'running', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$service_ensure = 'running', | |
String $service_ensure = 'running', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be an enum type (ie: running
without quotes) so I don't want to force the type. We do this in several other places in the module.
- Add manage_install parameter to disable installing the Agent - Split service and repo management, since service code is the same for all Linuxes while repo code is not.
manage_install
parameter to disable installing the AgentFixes: #476
Fixes: #604