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

Don't clobber other cookbook's attributes #26

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
7 participants
@ansel1
Copy link

ansel1 commented Oct 24, 2012

Fixes #23

comment out default values in attributes/default.rb. Instead of setting default values (which can override the real cookbooks' default values), be more paranoid about checking for existence of those nodes in the template.

Btw, there is another way to handle this:

If you prefer the template code to be simple and clean, without all the 'if' checks, you can create a new recipe in your cookbook which sets the value of these attributes to default values only if the attribute is not already set. Then include that recipe before you run the template.

The advantage of this approach (which is advocated by opscode: http://community.opscode.com/questions/77) is that it can be run either at recipe compilation or recipe convergence time, rather than during attribute compilation. This ensures that all cookbooks have set their up their attributes. Also, if cookbooks set attributes during the recipe convergence phase, this honors the run-list order: your recipe will see any attributes set by recipes executed before this one, which is the intuitive behavior.

ansel1 added some commits Oct 24, 2012

Stop clobbering other cookbook's attributes
Fixes #23

comment out default values in attributes/default.rb.  Instead of setting default values (which can override the real cookbooks' default values), be more paranoid about checking for existence of those nodes in the template.
This attribute isn't even set by mysql
It's a super node used by mysql for something else completely.  We really need our own setting here.
@ansel1

This comment has been minimized.

Copy link
Author

ansel1 commented Oct 24, 2012

ug, even this isn't right. The mysql attributes aren't even really overloading the mysql cookbook's attributes. Or rather, the mysql cookbook does use node['mysql']['server'], but not for the same thing as this cookbook. And I don't think it sets the user or password attributes at all.

datadog really should be using it's own namespace for attributes which are private to this cookbook. It should only be re-using other cookbook's attributes when it's interpreting those attributes the same...

@ansel1

This comment has been minimized.

Copy link
Author

ansel1 commented Oct 25, 2012

postgres is the same way. I'm with the other guy. These should be namespaced to datadog. They are unique semantically.

@ghost ghost assigned alq666 Nov 6, 2012

@mike-yesware

This comment has been minimized.

Copy link

mike-yesware commented Nov 16, 2012

+1 for moving all of them into the datadog namespace.

I'm using the Redisio (not Redis) cookbook, so I have to use a wrapper that transforms it's values into Redis values. Might as well just be explicit and use DataDog values.

@vaskas

This comment has been minimized.

Copy link

vaskas commented Jan 15, 2013

+1 here for the namespace. Having a conflict with redis cookbook at the moment.

@alq666

This comment has been minimized.

Copy link
Contributor

alq666 commented Jan 15, 2013

We hear you. attributes are going to be namespaced.

@scatterbrain

This comment has been minimized.

Copy link

scatterbrain commented Feb 1, 2013

+1. Had problems with Redis cookbook and spent significant time trying to figure out why. Would appreciate a fix.

@alq666

This comment has been minimized.

Copy link
Contributor

alq666 commented Feb 1, 2013

This is going to be merged into #43.

@psi

This comment has been minimized.

Copy link

psi commented Apr 2, 2013

+1 for moving all node attributes under the datadog namespace. That's a general expectation for well-behaving chef cookbooks, in my opinion.

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Apr 22, 2013

Hi @ansel1,
I believe this has been addressed in #46, and will be in an upcoming release.
Thanks for your efforts!

@miketheman miketheman closed this Apr 22, 2013

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.