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

fix role specification for elasticsearch and graphite #22

Merged
merged 1 commit into from Sep 16, 2014

Conversation

Projects
None yet
2 participants
@jwmarshall
Contributor

jwmarshall commented Sep 15, 2014

This change fixes the ability to dynamically configure the remote end points for graphite and elasticsearch. I could not figure out how the original method was supposed to work given that the es_server and graphite_server had default variables defined in attributes/default.rb

@JonathanTron

This comment has been minimized.

Collaborator

JonathanTron commented Sep 16, 2014

Thanks for the pull-request.
What is expected is that once you want to use the roles you set:

override['grafana']['es_server'] = nil
override['grafana']['graphite_server'] = nil

and either use the defaults node['grafana']['es_role']/node['grafana']['graphite_role'] or change them to match yours.

I think your change is fine and will apply it with a warning in the README that it will now search for the es_role/graphite_role by default even if you set the es_server/graphite_server and will replace the defaults.

JonathanTron added a commit that referenced this pull request Sep 16, 2014

Merge pull request #22 from BNOTIONS/master
fix role specification for elasticsearch and graphite

@JonathanTron JonathanTron merged commit 134878f into sous-chefs:master Sep 16, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jwmarshall

This comment has been minimized.

Contributor

jwmarshall commented Sep 16, 2014

Jonathan, that makes a little more sense as setting the default to nil did not work for me. I guess using override or normal even would have worked. However the override of each server to nil feels very strange when you're reading the environment/role attributes. I think the update you've merge makes the most sense, but I'm probably biased to that as I wrote the pull request.

Never-the-less thank you for a great cookbook and merging my updates. 👍

@lock

This comment has been minimized.

lock bot commented Jul 24, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jul 24, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.