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

Allow for a hash or string when configuring tags #296

Merged
merged 9 commits into from Apr 21, 2016

Conversation

Projects
None yet
2 participants
@martinisoft
Copy link
Contributor

martinisoft commented Apr 17, 2016

A pattern we're employing with our wrapper cookbooks is to compose the datadog tags attribute so they're rendered out properly in the datadog template. The way it currently is now, the template is rendered at compile time which prevents wrapper cookbooks from modifying the tags during run time. By extracting the attribute to a template variable, we delay the rendering of the template variable until the run phase.

With this patch you will be able to do the following in a recipe:

node.default['datadog']['tags'] = node['datadog']['tags'].merge('region' => 'us-east')

What this does is composes the current set of tags defined in the attribute with an additional tag by way of merging in a new key and value into the hash. You can still provide a string instead which is the currently shipped default so these changes are fully backward compatible.

Extract datadog tags into a template var
This might help with wrapper cookbooks using a composition pattern
@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Apr 17, 2016

Interesting. I thought the template variable was timed differently.
Had you seen the approach suggested here? #186

This might solve the problem a little more elegantly.

@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Apr 18, 2016

@miketheman Not entirely. A more flexible solution could be for adding a resource for configuring the agent that allows a wrapping cookbook to format the template however they choose.

@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Apr 19, 2016

@miketheman I've added support for a hash similarly to #186 with test coverage so it shouldn't be a breaking change.

@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Apr 19, 2016

I have updated the example in the initial post so this is ready for review and merge. 👍

@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Apr 20, 2016

Well this has come full circle now so this more or less duplicates the work in #186 though mine can be merged and passes CI.

@martinisoft martinisoft changed the title Extract datadog tags into a template var Allow for a hash or string when configuring tags Apr 20, 2016

@@ -31,6 +31,9 @@
default['datadog']['url'] = 'https://app.datadoghq.com'

# Add tags as override attributes in your role
# This can be a string of comma separated tags or a hash in this format:
# default['datadog']['tags'] = { 'datacenter' => 'us-east' }
# Thie above outputs a string: 'datacenter:us-east'

This comment has been minimized.

Copy link
@miketheman

miketheman Apr 20, 2016

Collaborator

nit: typo

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Apr 20, 2016

Thanks for your persistence! Can you add a spec test to cover the existing tags as a string behavior?

@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Apr 20, 2016

@miketheman done! Also added a bonus spec to cover an empty tag not being output.

@@ -15,7 +15,11 @@ autorestart: <%= node['datadog']['autorestart'] %>
skip_ssl_validation: <%= node['datadog']['web_proxy']['skip_ssl_validation'] %>
<% end -%>

<% if node['datadog']['tags'].respond_to?(:each_pair) -%>
tags: <%= node['datadog']['tags'].reject{|k,v| v.empty? }.map{|k,v| "#{k}:#{v}" }.join(',') %>

This comment has been minimized.

Copy link
@miketheman

miketheman Apr 21, 2016

Collaborator

Style nitpicks: spacing and unused block arg representation.
Should be node['datadog']['tags'].reject { |_k, v| v.empty? }.map { |k, v| "#{k}:#{v}" }.join(',')

@martinisoft

This comment has been minimized.

Copy link
Contributor Author

martinisoft commented Apr 21, 2016

@miketheman Done :)

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Apr 21, 2016

Thanks! Putting in merge queue.

@miketheman miketheman added this to the Next minor milestone Apr 21, 2016

@miketheman miketheman self-assigned this Apr 21, 2016

@miketheman miketheman merged commit a0fbc0e into DataDog:master Apr 21, 2016

2 checks passed

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

This comment has been minimized.

Copy link
Collaborator

miketheman commented Apr 21, 2016

@martinisoft Thanks for working through this! Now merged to master.

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.