wodby / varnish

Varnish docker container image
https://wodby.com/stacks/varnish
MIT License
59 stars 29 forks source link

Allow setting a custom backend storage #4

Closed esolitos closed 6 years ago

esolitos commented 6 years ago

With this change the secondary storage bin can be customised on demand, allowing to define, for example, another memory storage. This is can be particularly useful in conjunction with wodby/drupal-varnish#3 because it allows the bin to be completely customised.

Note This is an extension of VARNISHD_STORAGE_SIZE, making it more customisable while preserving backward compatibility.

csandanov commented 6 years ago

I think it would be better to use something like VARNISHD_SECONDARY_STORAGE where you can specify storage name, type, path and size.

esolitos commented 6 years ago

I agree, that VARNISHD_SECONDARY_STORAGE is a better name for the variable (changed it now), however I was thinking to leave the storage name fixed to secondary, just to prevent the need for a second variable for the storage name in the use-case: wodby/drupal-varnish#3 However if you think it's better leaving the storage name configurable as well, I'll change that as well.

csandanov commented 6 years ago

What I meant is to have just a single env var VARNISHD_SECONDARY_STORAGE instead of two env vars. As the example value it could be:

VARNISHD_SECONDARY_STORAGE=file,/var/lib/varnish/storage.bin,10MiB

Because now variable VARNISHD_STORAGE_SIZE actually means "add additionall storage with type file, named secondary, with the path /var/lib/varnish/storage.bin and with the size what have specified". We can keep name secondary because it will be used in the variable name.

esolitos commented 6 years ago

Ok, the idea was to leave VARNISHD_STORAGE_SIZE for backward compatibility, do you think I should simply drop it?

Last commits are just small changes: renamed the variable and changed the priority since the VARNISHD_STORAGE_SIZE would be deprecated.

csandanov commented 6 years ago

Ohh, I'm sorry, I didn't realize we had it before :) yes, I think we should drop it, it's stupid

esolitos commented 6 years ago

Updated dropping VARNISHD_STORAGE_SIZE in favour of the more generic VARNISHD_SECONDARY_STORAGE.