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

['rabbitmq']['config'] should not be hard coding again #155

Closed
wenchma opened this issue Oct 29, 2014 · 6 comments

Comments

Projects
None yet
3 participants
@wenchma
Copy link
Contributor

commented Oct 29, 2014

This line: https://github.com/jjasghar/rabbitmq/blob/master/attributes/default.rb#L19
default['rabbitmq']['config'] = '/etc/rabbitmq/rabbitmq'
I think it should use config_root attribute like this: default['rabbitmq']['config'] = "#{node['rabbitmq']['config_root']}/rabbitmq"

so we only need to configure the attribute ['rabbitmq']['config_root'] .

The same as line https://github.com/jjasghar/rabbitmq/blob/master/attributes/default.rb#L89

@jjasghar

This comment has been minimized.

Copy link
Collaborator

commented Nov 27, 2014

Interesting, what gets me is Generic UNIX - $RABBITMQ_HOME/etc/rabbitmq/ we have support for things other than Debian and Redhat here, so wouldn't that be the reason why we have it configurable?

@michaelklishin

This comment has been minimized.

Copy link
Member

commented Nov 27, 2014

Yes, it should be configurable. Instead, values that depend on it should be re-calculated if when the value changes. People may want to move configuration location for various reasons.

@wenchma

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2014

@jjasghar @michaelklishin
Yes, I ignored the Generic UNIX, it should be configurable, but I also have a little question.
This line: https://github.com/jjasghar/rabbitmq/blob/master/attributes/default.rb#L19
default['rabbitmq']['config'] = '/etc/rabbitmq/rabbitmq'
I think it should use config_root attribute like this: default['rabbitmq']['config'] = "#{node['rabbitmq']['config_root']}/rabbitmq"

so we only need to configure the attribute ['rabbitmq']['config_root'] .

@jjasghar

This comment has been minimized.

Copy link
Collaborator

commented Nov 29, 2014

@wenchma great catch, yeah there is no need for that hard coding twice.

Seems like a great opportunity for a PR 😉

@wenchma wenchma changed the title rabbitmq-env.conf should not be exposed to be configurable ['rabbitmq']['config'] should not be hard coding again Dec 1, 2014

@wenchma

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2014

I have updated the issue .

@jjasghar

This comment has been minimized.

Copy link
Collaborator

commented Dec 1, 2014

@wenchma awesome, we should have that merged as soon as one more 👍 happens on it.

@jjasghar jjasghar closed this Dec 2, 2014

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.