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

gmmeyer
Copy link
Contributor

@gmmeyer gmmeyer 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
Copy link
Member

degemer 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
Copy link
Contributor Author

gmmeyer 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
Copy link
Member

degemer 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
Copy link
Member

@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
Copy link
Contributor Author

gmmeyer 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
Copy link
Member

LGTM, thanks @gmmeyer!

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

Successfully merging this pull request may close these issues.

3 participants