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

Helpers should be in a cookbook-specific module #22

Closed
chewi opened this issue May 14, 2015 · 6 comments
Closed

Helpers should be in a cookbook-specific module #22

chewi opened this issue May 14, 2015 · 6 comments
Assignees

Comments

@chewi
Copy link
Contributor

chewi commented May 14, 2015

I think it is bad practise to define methods directly in Chef::Provider. You should define them in a module specific to this cookbook and then include them in the providers. It would also allow community cookbook writers to put this in their recipes.

include SELinuxPolicy::Helpers
include_recipe 'selinux_policy::install' if use_selinux

They wouldn't want to include the install recipe unconditionally. If they don't include it at all, the resources will break when SELinux is enabled but the packages are missing.

@BackSlasher
Copy link
Contributor

The first part I agree with - I merged this when I wasn't that familiar with Ruby, and I will seperate to a different module.
I don't think I should leave the mixins to the cookbook users - I'll play around with using extend at the instance level, like this:

action :add do
  escaped_file_spec = Regexp.escape(new_resource.file_spec)
  execute "selinux-fcontext-#{new_resource.secontext}-add" do
    extend SELinuxPolicy::Helpers
    command lazy { "/usr/sbin/semanage fcontext -a -t #{new_resource.secontext} '#{new_resource.file_spec}' && #{restorecon(new_resource.file_spec)}" }
    not_if "/usr/sbin/semanage fcontext -l | grep -P '#{escaped_file_spec}\s.*\ssystem_u:object_r:#{new_resource.secontext}:s0'>/dev/null"
    only_if {use_selinux}
  end
end

@BackSlasher BackSlasher self-assigned this May 15, 2015
@chewi
Copy link
Contributor Author

chewi commented May 15, 2015

That doesn't help the situation with the install recipe though. Calling include in your recipe isn't an unusual thing to do but I did get it slightly wrong above.

::Chef::Recipe.send(:include, SELinuxPolicy::Helpers)
include_recipe 'selinux_policy::install' if use_selinux

Not quite as nice but you can see this technique referenced here. Using extend instead does work though as explained here.

extend SELinuxPolicy::Helpers
include_recipe 'selinux_policy::install' if use_selinux

@BackSlasher
Copy link
Contributor

I thought you were talking about how I'm using it in my own recipe. I now think you're referring to how you're utilizing it in your own recipes.
Are you suggesting creating a recipe that does the mixin, so consumers can easily mixin for their own use, and avoid the mixin automatically (in case it breaks something elsewhere)?

@chewi
Copy link
Contributor Author

chewi commented May 15, 2015

I was referring to how you are using it in your providers but also how it could be used in recipes for other cookbooks. I'll submit a pull request later to ensure we're on the same page.

@BackSlasher
Copy link
Contributor

Great :)
I'm onboard for making the cookbook cleaner. Not sure about the mixin
recipe though.
On May 15, 2015 6:36 PM, "James Le Cuirot" notifications@github.com wrote:

I was referring to how you are using it in your providers but also how
it could be used in recipes for other cookbooks. I'll submit a pull request
later to ensure we're on the same page.


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

@lock
Copy link

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.
Labels
None yet
Development

No branches or pull requests

2 participants