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

Open file limit not set correctly #127

Merged
merged 16 commits into from Apr 23, 2015

Conversation

jessedavis
Copy link
Contributor

If node['rabbitmq']['open_file_limit'] is set to anything other than the default, the resulting ulimit line is normally written in /etc/rabbitmq/rabbitmq-env.conf. That file is usually sourced, and doing so usually prints out an error along the lines of "operation not permitted" when using rabbitmqctl, etc.

The correct place for the ulimit is in /etc/default/rabbitmq-server. I tested this manually by applying the updated recipe to the default-centos-65 and default-ubuntu-1204 images.

I followed the same format for the minitest tests, but could not figure out how to execute them. I'd appreciate a quick rundown on how that's supposed to work, as I'm pretty new to Chef and test-kitchen and am just learning them now.

@jjasghar
Copy link
Contributor

jjasghar commented Dec 7, 2014

Can you rebase this off master and see if it's still relevant? Thanks!

@jjasghar jjasghar added TODO and removed TODO labels Dec 7, 2014
@@ -167,6 +167,14 @@ class Chef::Resource # rubocop:disable all
notifies :restart, "service[#{node['rabbitmq']['service_name']}]", :immediately
end

template '/etc/default/rabbitmq-server' do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use template "/etc/default/#{node['rabbitmq']['service_name']}" instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@stevecstian
Copy link

@jessedavis
Thank you for your contribution on this. Not sure if you have time to do some minor improvements per above line notes?

@jessedavis
Copy link
Contributor Author

Yup! I'll try to carve out some time soon to get those folded in; thanks for all the review notes!

@jessedavis
Copy link
Contributor Author

This should be ready for re-review.

@stevecstian
Copy link

👍 @jjasghar @jessedavis Do we still need a spec for this?

@jjasghar
Copy link
Contributor

Yeah i'd like a added test in the default_spec.rb at least for that additional template resource.

@jessedavis
Copy link
Contributor Author

@jjasghar @stevecstian I added a spec (my first time using ChefSpec), so please feel free to reject if I did it wrong. I did have to upgrade ChefSpec above 4.0 to get the tests to start. Since this is my first time contributing to and using Chef, is it normal best practice to perform bundle upgrade frequently to pull in updated dependencies?

source 'default.rabbitmq-server.erb'
owner 'root'
group 'root'
mode 00644
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a 4 digit string: '0644'

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 5 digit should be fine. I believe this just forces the octet rule

@stevecstian
Copy link

@tas50 It should be fine to specify 5 digit integer for the value of mode attribute as we're using that format in some places of default recipe.

@@ -8,7 +8,7 @@ end

group :unit do
gem 'berkshelf'
gem 'chefspec'
gem 'chefspec', '~> 4.0'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjasghar Is this required?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paging @jjasghar
Will Gemfile.lock help on this?
/CC @michaelklishin

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh, you should be running the newest version of chefspec right?

@jessedavis
Copy link
Contributor Author

@jjasghar I think this PR should be ready for a merge (sorry about the long delay).

@jjasghar
Copy link
Contributor

@jessedavis we've made some comments on the PR, can you either respond or update the PR with those changes? Thanks!

@jessedavis
Copy link
Contributor Author

@jjasghar I believe I've made all the changes that you and @stevecstian recommended in the comments. Let me know if I'm mistaken or if I didn't make the changes correctly.

@johnbellone
Copy link

👍

1 similar comment
@ikurochkin
Copy link

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants