Added a define that wraps the agent integration command

[albertvaka]

No description provided.

[truthbk]

Thanks a bunch for doing this, I left some feedback but we're on a great track :)

[truthbk]

Shouldn't ~> 4.6.0 basically give you the upper bound on 4.6.x ?

[albertvaka]

'~> 4.6.2' == '4.6.2 < version < 4.7.0'
'~> 4.6.0' == '4.6.0 < version < 4.7.0'

I didn't assume it works on 4.6.0 because on Travis we test with 4.6.2.

[truthbk]

This should probably require something like:

    require => Package[$datadog_agent::params::package_name],

We probably don't need to notify, we can leave that to the config file definition.

[albertvaka]

👍

[truthbk]

We should probably state here how the installation of the integration should happen in the manifests before the config file definition for the integration.

[albertvaka]

Does the config file definition need the integration to be installed in order to generate the config file?

[truthbk]

No, it's not needed, but since we're not restarting the agent here - we'd be relying on the config file for that, and steps would need to be in order (actually this might not be entirely true if the config files trigger the service restart in a delayed fashion). Alternatively, maybe ideally (though I'm not sure because technically speaking installing an integration does not imply it's enabled), we could notify the service to restart here, maybe in a delayed fashion. And then we could forget any changes to the docs, as they'd be moot.

[truthbk]

This has a slight problem, given that if a customer installs an integration, this will install it with every run, making the module chatty. Ideally we'd want to only add the resource if the integration has not been installed (same goes for absent).

You might be able to check for the existence of /opt/datadog-agent/embedded/lib/python<PYVERSION>/site-packages/datadog_<integration>-<version>.dist-info to decide if you go forward with the execution or not. Or you could ask pip.

[albertvaka]

The integration command is idempotent, and exec only prints if the command ends with a non-zero status (so it won't make extra work and it won't be chatty).

I find it way cleaner to rely on the integration command doing the check than us digging into some (version-dependent) path to check for some internal pip files.

[truthbk]

I wasn't sure if integration was idempotent, it's good to hear it is. I think this would still make the module chatty though, signaling resource changes when no real resource has changed or should've been changed. If you can verify it's not the case, then I naturally don't see a need to change anything - but if it does, we probably should reconsider the approach.

[truthbk]

How does the tool handle python2, python3 integration installations? I presume it installs for the python specified in the datadog.yaml.... Let's keep that in mind as a use case for the module, particularly when you check for an integration's presence.

[albertvaka]

I think it does what you say, but in any case puppet should just assume the integration command does the right thing when multiple pythons are installed.

[truthbk]

This is true if we rely on the integration command entirely, but we first need to make sure the module isn't chatty before going for this approach.

[albertvaka]

Any comments about the name? I couldn't think of anything better than install_integration, but let me know if you have any ideas.

[truthbk]

I think this is good for 🚀