search

voxpupuli / puppet-squid

Puppet module for configuration of squid caching proxy.
https://forge.puppet.com/puppet/squid
Other
12 stars 54 forks source link

Validate squid config before applying changes #122

Closed jocelynthode closed 5 years ago

jocelynthode commented 5 years ago

Pull Request (PR) description

This PR aims to make use of the validate_cmd parameter from concat/file so that the squid configuration file is checked before being applied.

This prevent Squid from catastrophically failing when a bad change is pushed.

P.S.: The change is at the moment quite simple, However if you want me to add a variable for the validate_cmd, I can do this as well.

traylenator commented 5 years ago

Good idea!

traylenator commented 5 years ago 
  Error: Execution of '/usr/sbin/squid -k check -f /etc/squid/squid.conf20190411-2128-1t8n48a' returned 1: squid: ERROR: No running copy

from the tests.

jocelynthode commented 5 years ago

@traylenator Yeah I wrongly used check instead of parse. I have to test some more.

Do you also want me to add more tests ?

alexjfisher commented 5 years ago

/usr/sbin/squid -k parse -f ... instead?

-k reconfigure | rotate | shutdown | interrupt | kill | debug | check | parse Parse configuration file, then send signal to running copy (except -k parse ) and exit.

jocelynthode commented 5 years ago

As I'm not familiar with FreeBSD I do not know how to verify if /usr/sbin/squid is correct for it. Do you have any tips for this ?

Is there some more tests that you'd like me to add ? I did not see any documentation of parameters so I did not add one. If I need to add some doc strings let me know :)

Edit: added some doc in the README for the bin_path parameter

jocelynthode commented 5 years ago

I switched to squid_bin_path from bin_path thanks to @ekohl suggestion

jocelynthode commented 5 years ago

@alexjfisher Hey, thanks for the review! The changes you requested have been pushed. Do you have an ETA on when this can be merged and when can we expect to see this change on the forge ?

jocelynthode commented 5 years ago

@alexjfisher Is there something you want me to change ?

alexjfisher commented 5 years ago

@jocelynthode I think I pinged you about this on slack?? I self assigned this as I was going to push a very small change, rebase/squash and then merge. You'd unticked allow edits from maintainers though, so I couldn't.

alexjfisher commented 5 years ago

Superceded by and merged in https://github.com/voxpupuli/puppet-squid/pull/123

jocelynthode commented 5 years ago

@alexjfisher Hey, Sorry I'm not really using slack. I did not even remember I had an account on it. Did you direct message me on Slack or am I in a channel ? Too bad it was not my pr that was merged but oh well! Next time I'll open one I'll make sure to let maintainers edit my PR to make the collaboration easier.

Thanks for the review and the merge !

alexjfisher commented 5 years ago

It was on the #voxpupuli channel.

jocelynthode [2:33 PM] joined #voxpupuli by invitation from afisher. afisher [2:48 PM] @jocelynthode I had a very minor change I was going to make to your squid PR just before squashing and merging. Can you enable 'Allow edits from maintainers.' on that PR? (it's tidier than me opening a new PR and easier than nagging you to make the change and squashing - but we can do that instead if you prefer)

You're still credited as the author on the commit. https://github.com/voxpupuli/puppet-squid/pull/123/commits/bd582a28f1158992aaef498a024d21f5898ec1c9