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

* add support for looback users #212

Merged
merged 10 commits into from Feb 5, 2015

Conversation

Projects
None yet
5 participants
@sethcall
Copy link
Contributor

commented Feb 2, 2015

Pretty straightforward change

@jjasghar

This comment has been minimized.

Copy link
Collaborator

commented Feb 2, 2015

Any chance you can get a chefspec test in for this? Something like: https://github.com/jjasghar/rabbitmq/blob/master/spec/default_spec.rb#L94-L97 and making sure it shows up as expected?

@sethcall

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2015

I have not done a chef_spec. I'll see if I can get this going...

@jjasghar

This comment has been minimized.

Copy link
Collaborator

commented Feb 2, 2015

Yeah, i respect that. It's a good practice to have, and i'm trying to enforce it for any PRs from the travis build on.

@sethcall

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2015

Tests are in

@jjasghar jjasghar removed the needs tests label Feb 2, 2015

@michaelklishin

This comment has been minimized.

Copy link
Member

commented Feb 3, 2015

👍

@stevecstian

This comment has been minimized.

Copy link

commented Feb 3, 2015

@sethcall @jjasghar @michaelklishin

How about having loopback_users as array like below?

attributes/default.rb:

# rabbitmq.config defaults
default['rabbitmq']['default_user'] = 'guest'
default['rabbitmq']['default_pass'] = 'guest'

# List of users which are only permitted to connect to the broker via a loopback interface (i.e. localhost).
default['rabbitmq']['loopback_users'] = [ 'guest' ]

templates/default/rabbitmq.config.erb

     {default_user, <<"<%= node['rabbitmq']['default_user'] %>">>},
     {default_pass, <<"<%= node['rabbitmq']['default_pass'] %>">>},
<% if node['rabbitmq']['loopback_users'] -%>
    {loopback_users, [<%= node['rabbitmq']['loopback_users'].map{|n| "<<\"#{n}\">>"}.join(',') %>]},
<% end %>
     {heartbeat, <%= node['rabbitmq']['heartbeat'] %>}

You might want to test:

  • attribute value is nil
  • single loopback user
  • multiple loopback users

Consider to add some descriptions for this new attribute in README.md or even metadata.rb.

@sethcall

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2015

Yes good point. I'll make that change

On Mon, Feb 2, 2015 at 8:27 PM, stevecstian notifications@github.com
wrote:

@sethcall https://github.com/sethcall @jjasghar
https://github.com/jjasghar @michaelklishin
https://github.com/michaelklishin

How about having loopback_users as array like below?

attributes/default.rb:

rabbitmq.config defaults

default['rabbitmq']['default_user'] = 'guest'
default['rabbitmq']['default_pass'] = 'guest'

List of users which are only permitted to connect to the broker via a loopback interface (i.e. localhost).

default['rabbitmq']['loopback_users'] = [ 'guest' ]

templates/default/rabbitmq.config.erb

 {default_user, <<"<%= node['rabbitmq']['default_user'] %>">>},
 {default_pass, <<"<%= node['rabbitmq']['default_pass'] %>">>},

<% if node['rabbitmq']['loopback_users'] -%>
{loopback_users, [<%= node['rabbitmq']['loopback_users'].map{|n| "<<"#{n}">>"}.join(',') %>]},
<% end %>
{heartbeat, <%= node['rabbitmq']['heartbeat'] %>}

You might want to test:

  • attribute value is nil
  • single loopback user
  • multiple loopback users

Consider to add some descriptions for this new attribute in README.md or
even metadata.rb.


Reply to this email directly or view it on GitHub
#212 (comment).

sethcall added some commits Feb 3, 2015

@sethcall

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2015

@stevecstian I went with all of your suggestions; I also added a default value in the attributes/default.rb file to let people know the config is available (I almost always go there 1st when learning any cookbook).

The readme has no mention of any attributes/default.rb, so I added default_user and default_pass, along with loopback_users... (it would have been weird to have loopback_users all by itself).

EDIT: I just noticed you suggested a default of ['guest'] for the value. That'd be OK too, I suppose, but I think I prefer this cookbook not writing values into the config by default. (mimicking a minimal config that you'd have with rabbitmq when installed via package)

@stevecstian

This comment has been minimized.

Copy link

commented Feb 3, 2015

@sethcall You ROCK 🤘 !
I'd give this 👍 👍

I just realized that we have 2 different places to describe attributes:

  • metadata.rb: description, type, default, ...
  • README.rb: usage, ...

I'm fine to have a sub-section in README.rb for attributes. But I think @jjasghar can make a final call on this.

I'm OK to have nil as the default value of loopback_users. The only reason I prefer to use ['guest'] as default value because user can easily realize we should assign an array for this attribute. But I guess that's not really a big deal...

@jjasghar @michaelklishin Any suggestions?

/CC @tsupertramp for similar fix in #206

@thomaspeitz

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2015

@sethcall Yeah! Thanks!

sethcall added some commits Feb 3, 2015

@sethcall

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2015

I didn't even look at metadata.rb. I've updated it to keep in line with the other attributes.

I think your point about specifying an array of users is smart. I tried to address your concern in the readme, by making a specific example of specifying a user.

sethcall added some commits Feb 3, 2015

@stevecstian

This comment has been minimized.

Copy link

commented Feb 3, 2015

👍

jjasghar pushed a commit that referenced this pull request Feb 5, 2015

JJ Asghar
Merge pull request #212 from sethcall/master
*  add support for looback users

@jjasghar jjasghar merged commit df4c8e3 into rabbitmq:master Feb 5, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
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.