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

[chef] adds hostname to the arguments passed to the gem #308

Merged
merged 2 commits into from Jun 14, 2016

Conversation

Projects
None yet
3 participants
@gmmeyer
Copy link
Member

commented May 9, 2016

Currently, the hostname isn't being passed to the gem. It needs to be in order for the hostnames to be sync'd up on the serverside (if you're using friendly hostnames).

@degemer

This comment has been minimized.

Copy link
Member

commented May 23, 2016

To be sure, node['datadog']['hostname'] is the hostname used by the agent ?

Could there be a edge case where a customer is using only dd-handler without dd-agent, and this PR would change in a wrong way the hostname ? (because this PR means that https://github.com/DataDog/chef-handler-datadog/blob/master/lib/chef/handler/datadog.rb#L98-L99 shouldn't be called anymore)

@gmmeyer

This comment has been minimized.

Copy link
Member Author

commented May 23, 2016

@degemer no this was just for the handler, the bug only affected the handler. So they were getting two servers when the handler ran where they should have had only one.

@degemer

This comment has been minimized.

Copy link
Member

commented May 23, 2016

@gmmeyer It makes sense, and anyway if hostname is defined it should be used. :shipit:

@olivielpeau olivielpeau self-assigned this Jun 1, 2016

@olivielpeau

This comment has been minimized.

Copy link
Member

commented Jun 8, 2016

@gmmeyer: Although I agree that it makes sense for the dd-handler recipe to force the handler to use the hostname that's defined, I'm slightly worried about the silent change to the cookbook's behavior that this introduces.

For instance, if node['datadog']['hostname'] is left to its default value and node['datadog']['use_ec2_instance_id'] is set to true, the handler would start reporting with node.name instead of the ec2 instance id.

So I'm thinking this could be regarded as a BC breaking change and may have to wait until the next major release of the cookbook.

I've implemented the same change with a slightly different approach (with an additional boolean attribute) in #290. My approach is definitely a bit convoluted though. Let me know what you think.

cc @degemer

@gmmeyer

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2016

@olivielpeau that's a great point. Largely inspired by your fix, I fixed it, in a slightly less convoluted way than your fix, I think.

@olivielpeau

This comment has been minimized.

Copy link
Member

commented Jun 14, 2016

LGTM, thanks @gmmeyer!

@gmmeyer gmmeyer merged commit fdc80b4 into master Jun 14, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@olivielpeau olivielpeau deleted the greg/chef-hostname-issue branch Aug 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.