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

Remove DD_HOSTNAME from agent and move it back to check runner #139

Conversation

carloscastrojumo
Copy link
Contributor

@carloscastrojumo carloscastrojumo commented Aug 27, 2020

What does this PR do?

Remove DD_HOSTNAME as environment variable from DataDog agent and put it back to check runner.

Motivation

DD_HOSTNAME was removed from check runner and added by mistake in the agent in this PR #3

Additional Notes

Add dependency for bin/operator-sdk in make container and make e2e commands.

@carloscastrojumo carloscastrojumo requested a review from a team as a code owner August 27, 2020 15:11
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2020

Codecov Report

Merging #139 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #139   +/-   ##
=======================================
  Coverage   58.02%   58.02%           
=======================================
  Files          31       31           
  Lines        4610     4610           
=======================================
  Hits         2675     2675           
  Misses       1739     1739           
  Partials      196      196           
Flag Coverage Δ
#unittests 58.02% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/controller/datadogagent/utils.go 83.16% <ø> (-0.12%) ⬇️
pkg/controller/datadogagent/clusterchecksrunner.go 72.63% <100.00%> (+0.63%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf31195...445bc70. Read the comment docs.

@ahmed-mez
Copy link
Contributor

Hi @carloscastrojumo thanks for the contribution.

Actually DD_HOSTNAME should be set explicitly only for the datadog-cluster-checks-runner. (see the helm chart for reference)

We shouldn't set the hostname for the datadog-agent and it's the case currently because of an oversight I believe, introduced by mistake in the refactoring done by this PR #3

The agent has automatic mechanisms to discover the node name, the option should be only used in particular cases - that's why the option exists.

Would you be interested in fixing it by removing DD_HOSTNAME from the agent, and move it to the check runner only?

Thanks!

@ahmed-mez ahmed-mez added the bug Something isn't working label Aug 27, 2020
@ahmed-mez ahmed-mez added this to the v0.4 milestone Aug 27, 2020
@carloscastrojumo
Copy link
Contributor Author

Hi @ahmed-mez thank you for the review.

As suggested, updated my PR to remove the DD_HOSTNAME from agent and cluster agent and just introduce it back to check runner.

@ahmed-mez
Copy link
Contributor

@carloscastrojumo that's great! could you please update the PR's title and description accordingly? thanks!

@carloscastrojumo carloscastrojumo changed the title Add DD_HOSTNAME to envs vars in cluster-agent and checks-runner Remove DD_HOSTNAME from agent and move it back to check runner Aug 27, 2020
@ahmed-mez ahmed-mez merged commit 3b8bf72 into DataDog:master Aug 27, 2020
@ahmed-mez ahmed-mez modified the milestones: v0.4, v0.3 Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants