Conversation
truthbk
left a comment
truthbk
left a comment
There was a problem hiding this comment.
Looks good for the most part, just a small comment. We'd also be missing the spec tests which are really must before we can merge. You should be able to get plenty of inspiration from the existing tests.
Thanks for this 🙇
| $dst = "${datadog_agent::conf_dir}/kubernetes.yaml" | ||
| } | ||
|
|
||
| package { 'dd-check-kubernetes': |
There was a problem hiding this comment.
This is not always true, we bundle a check with the agent, so what you should ensure here is the agent is installed not dd-check-kubernetes (I think that's actually taken care of by the include datadog-agent directive above).
| # Sample Usage: | ||
| # | ||
| # class { 'datadog_agent::integrations::kubernetes' : | ||
| # api_server_url => 'https://kubernetes:443', |
There was a problem hiding this comment.
You'll want to cover auth as well the the certificate options in kubernetes.yaml as well
| if $::datadog_agent::agent6_enable { | ||
| $dst = "${datadog_agent::conf6_dir}/kubernetes.yaml" | ||
| } else { | ||
| $dst = "${datadog_agent::conf_dir}/kubernetes.yaml" |
There was a problem hiding this comment.
Dont forget kube_state_metrics as well.
There was a problem hiding this comment.
This is technically a "different" integration, would need it's own manifest.
There was a problem hiding this comment.
Ah I see. I'll add the manifest for it.
|
Have you considered enabling autodiscovery here as well? service_discovery_backend: docker should be set, unless they override with one of the others. |
truthbk
left a comment
truthbk
left a comment
There was a problem hiding this comment.
Spec tests are still missing... could you please add them? 🙇
| @@ -0,0 +1,22 @@ | |||
| init_config: | |||
There was a problem hiding this comment.
kubernetes_state is a different integration, if you want to add this you'll need to have a separate manifest for it: manifests/integrations/kubernetes_state.pp
There was a problem hiding this comment.
Ah maybe I miss understood @irabinovitch request to not forget kubernetes_state_metrics. I reviewed the dd docs, but nothing states what is added to the yaml. Only the kubernetes pods file.
There was a problem hiding this comment.
I have added the tests, but these are my first spec tests ever. So please let me know if I miss understood anything or what to rework.
| # | ||
| # init_retries: 5 | ||
|
|
||
| instances: |
There was a problem hiding this comment.
I don't think there's a use case where we'd want to support multiple instances for the kubernetes integration... Is that correct? Just double-checking in case you want to add the support for that now. It's no biggie, as it's fairly easy to add (maybe not in the most idiomatic way) while retaining backward compatibility, we can always add it in the future.
There was a problem hiding this comment.
Ill just leave that out for now then.
|
merged #369 based off this PR and with spec tests fixed. Closing. Thanks @lowkeyshift |
2 participants
This will allow you to
include "datadog_agent::integrations::kubernetes".