Skip to content
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

Make sure checks.d directory is present #213

Closed
wants to merge 1 commit into from
Closed

Make sure checks.d directory is present #213

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 20, 2017

What does this PR do?

It makes sure the checks.d directory is present in the container.

Motivation

Otherwise the following copy command will create a file called 'checks.d' for the first check file.

We weren't previously able to mount a checks.d directory as a volume. The mounting-as-a-volume part worked, but the copying-to-the-actual-directory part was broken because the target checks.d directory within the container didn't exist.

Otherwise the following copy command will create a file
called 'checks.d' for the first check file.
@xvello xvello self-assigned this Jul 7, 2017
@xvello xvello added the bugfix label Jul 7, 2017
Copy link
Contributor

@xvello xvello left a comment

Choose a reason for hiding this comment

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

Hi @lgierth and thank you for your contribution. Indeed this is a bug in the current entrypoints.

  • To be consistent with how we handle /conf.d, I'd go with
    find /checks.d -name '*.py' -exec cp --parents {} /opt/datadog-agent/agent \;
    instead. This makes cp create the parent folder if needed (and it is indeed needed). Would that be OK with you?

  • You PR patches two entrypoints, but we have a third one in jmx/entrypoint.sh. Could you please include this one as well?

Regards

@aerostitch
Copy link
Contributor

@xvello I noticed this issue during the refactoring of the entrypoints in #222 and am fixing it there with --parents. (just FYI).

@ghost
Copy link
Author

ghost commented Aug 10, 2017

Hey sorry I totally forgot about this -- crazy busy times right now. @aerostitch thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants