chr4-cookbooks / iptables-ng

Cookbook to maintain iptables rules and policies on different platforms, respecting the way the os handles these settings.
GNU General Public License v3.0
38 stars 32 forks source link

silence chef 12 warnings #61

Closed sspans closed 8 years ago

sspans commented 8 years ago

This resolves resource cloning warnings reported by newer chef versions.

chr4 commented 8 years ago

Catching the exception seems like an interessting approach. I haven't seen it anywhere used before, is this a best practise?

Regarding some warnings, I filed a bug report against chef recently.

sspans commented 8 years ago

It seemed like the easiest way to handle the exception raised by looking for a non-existent resource.

Feel free to rework/re-implement this, but a solution would be nice because the warnings are duplicated for every chain and table. It makes chef runs quite messy.

chr4 commented 8 years ago

Yes, the warnings are super annoying, I agree. Thanks for your work on this!

Mmh, it's also possible to only catch the exception regarding the non-existent resource, not all of them, am I right? This would then have less impact on (potential) other exceptions.

Let me think a bit about whether catching the exception is without side-effects. Did you by chance run the tests?

lamont-granquist commented 8 years ago

you should be explicit and rescue Chef::Exceptions::ResourceNotFound

FWIW in recent Chef 12 you can:

find_resource(:directory, "/etc/iptables.d/#{new_resource.table}/#{new_resource.chain}" do
  owner  'root'
  group  node['root_group']
  mode   0o700
  not_if { exec_action == :delete }
end

which does exactly what you're doing with the search-for-the-resource-rescue-the-exception-and-declare-it pattern.

code is here: https://github.com/chef/chef/blob/23de6b876497a1a881d836700513eda3e85a1b59/lib/chef/dsl/declare_resource.rb

chr4 commented 8 years ago

Thanks for your feedback on this, Lamont! I suppose with the adapted rescue, this seems to be the best solution for now.

chr4 commented 8 years ago

@sspans Can you please fix the whitespace and --amend the latest commit?

providers/chain.rb:44:44: C: Trailing whitespace detected.
  rescue Chef::Exceptions::ResourceNotFound 

Besides that, looks good to me! Thanks for looking into this. Would you mind being added the the contributors list in the README?

lamont-granquist commented 8 years ago

it is in fact the /correct/ way to fix it -- the 3694 errors look like they were the root cause of the problem which is what was trolling you with the validation errors because you were re-opening and cloning the old resource to a new one all the time -- there's a bug there in core chef to just nuke resource cloning for orbit, but you still wouldn't want to have the duplicated resources even after we do that. this is, in fact, what you want. the only refinement would be to use the newer APIs, but if you want to support old cookbooks you either have to depend on compat_resource to pull in the new APIs on top of old chef, or just roll it yourself like this...

sspans commented 8 years ago

@chr4 whitespace error corrected. I wouldn't mind being listed as a contributor.

chr4 commented 8 years ago

@lamont-granquist for the record, I'm still getting the regex errors mentioned in https://github.com/chef/chef/issues/5090

chr4 commented 8 years ago

Nevermind, there was a rescue missing in the rule provider. This fixes it

chr4 commented 8 years ago

Released in v2.3.0.