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

Add support for multiple 'proxy_cache_valid' directives #788

Merged
merged 2 commits into from
Apr 12, 2016

Conversation

hbog
Copy link
Contributor

@hbog hbog commented Mar 25, 2016

Nginx supports multiple proxy_cache_valid directives.
This allows to specify a different caching time for different response codes.
(http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_cache_valid)

This PR adds the possibility to specify an array of strings for the 'proxy_cache_valid' attribute, while preserving backwards compatibility by still accepting a string (which is the expected behavior today).

Also, a unit test has been added that covers the new behavior, hence both the current and the new behavior is tested.

@3flex 3flex added the enhancement New feature or request label Apr 11, 2016
@3flex
Copy link
Contributor

3flex commented Apr 11, 2016

Looks like by using validate_slength with 12 as the value of the second argument that strings are limited to a maximum length of 12 characters. Seems this could easily be exceeded.

Is there a better way to validate the array values are all strings? Sure you can increase the character limit but it seems like a workaround.

@hbog
Copy link
Contributor Author

hbog commented Apr 11, 2016

The maximum length is indeed an arbitrary number and 12 may be too small. Increasing the number helps. A string longer than 64 characters for example is extremely unlikely for this directive.

An alternative is to stick to validate_array, which is also used elsewhere in the module to validate parameters containing an array of strings. This has the benefit of consistency with the rest of the code.

If you agree, I will replace validate_slength by validate_array.

@3flex
Copy link
Contributor

3flex commented Apr 11, 2016

validate_array doesn't add much value here though if you're using any2array, since we already know the output of that function is an array.

Ideally you would check the input is either a string or an array first, then either use any2array to ensure a string is turned into an array so the template works properly, or handle either a string or an array of strings in the template.

Whichever results in cleaner code is best but either way is OK with me.

@hbog
Copy link
Contributor Author

hbog commented Apr 12, 2016

The validation now checks for string or array first, using a pattern that I borrowed from similar validations elsewhere in the module.

@3flex 3flex merged commit fe0af9e into voxpupuli:master Apr 12, 2016
slm0n87 pushed a commit to slm0n87/puppet-nginx that referenced this pull request Mar 7, 2019
Add support for multiple 'proxy_cache_valid' directives
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
Add support for multiple 'proxy_cache_valid' directives
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants