-
-
Notifications
You must be signed in to change notification settings - Fork 883
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
Allow better control of http level proxy directives #642
Conversation
I'd really like to avoid adding parameters when it's not necessary, to prevent the module from bloating. Removal of conf.d/default.conf was added to address #263. What's the use case when you want to include that file? Could you instead create a new vhost for whatever configuration you have going into that file? For conf.d/proxy, I don't see any sense in having that in a separate file at all. What I'd do is move that file's contents into nginx.conf, and then add |
I don't care so much about default.conf, based on the referenced issue, I won't pursue that further. That makes sense. For proxy.conf, I would definitely prefer to clear it from conf.d. I can change my pull to move those params to change entries in nginx.conf, with the mentioned conditionals. |
4848dc0
to
9eb5460
Compare
Not sure if I should ensure proxy.conf gets deleted or not. But it will require manual intervention if I didn't, so I included the require=>absent. I added if clauses for every proxy variable and fixed some blank lines that were getting shoved in around the proxy header declarations. |
Looks great. Are you able to adjust the tests so they pass? You'll have to move the tests from https://github.com/jfryman/puppet-nginx/blob/master/spec/classes/config_spec.rb#L407-L451 to somewhere in the block starting at https://github.com/jfryman/puppet-nginx/blob/master/spec/classes/config_spec.rb#L78 And also remove the check for the proxy.conf file. |
Yep, I'll do that, likely on Monday. |
Fixed the tests to pass. I noticed that the way in which whitespace is handled is different in several cases. I had it match the exact value instead of regex to ignore it. Some of the proxy variables were never being tested for. Not sure if that is intentional or not. These aren't being tested in the spec, and were not prior to my change: |
That's great, thanks! We should have tests for those 4 parameters, but since we don't have them now there's not much point holding up the merge. Cheers! |
Allow better control of http level proxy directives
Allow better control of http level proxy directives
This parameter has no effect as of voxpupuli#642
Allow better control of http level proxy directives
This parameter has no effect as of voxpupuli#642
Understood if there is a particular reason control is being forced over these two files, but I have a use case where a flag would be nice to have for controlling management over these two files.
Only added the variables to config.pp because based on the deprecation warning, that's how it's suppose to be going forward.