-
-
Notifications
You must be signed in to change notification settings - Fork 881
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
Add all gzip params for http context #367
Conversation
…he erb template tests against to determine default overrides
don't forget to update the relevant tests for each class (under spec/classes) |
@rabbitt all done. There could be improvements in the passing of parameters to ensure strings/ints/arrays but the current modifications, albeit very infant, achieve the main goal of being able to add gzip settings to nginx.conf |
👍 |
{ | ||
:title => 'should set gzip_disable', | ||
:attr => 'gzip_disable', | ||
:value => 'MSIE [1-6]\.(?!.*SV1)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should test with a value different from the default to make sure it's being passed through correctly.
Looks awesome! If we can update the tests as mentioned that would be great. Parameter validation would also be a nice to have but it's not essential. Finally would you mind rebasing to tidy up the commits a bit? Squashing to a single commit would be fine. |
++ |
1 similar comment
++ |
+1 ... will this be merged? |
Not as is... there are merge conflicts and a few updates to be made. If someone wants to create an updated PR we can look at merging that instead, if @Richdel doesn't want to update this. |
Added all the available gzip params for the nginx.conf template and blank defaults which won't get picked up if not set therefore not written to .conf and nginx will use internal defaults. gzip_types param can be either a string or array.