-
Notifications
You must be signed in to change notification settings - Fork 9
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
Feature/templates configurable easy overwrite #23
base: master
Are you sure you want to change the base?
Conversation
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.
Hi @leonhelmus I've some small feedback, can you check them?
|
||
### Changed | ||
- Updated vcl file for varnish 6.x. | ||
- Now `nginx-map-dev.conf`, `.env.dev.project` do net get overwritten once |
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.
net = not.
"do net get overwritten once" means they won't get overwritten when they exist?
@@ -28,9 +28,16 @@ x-custom: | |||
``` | |||
Whenever you update a template in a project make sure you revert the custom changes. | |||
|
|||
|
|||
Custom changes can now be added in the `.env.dev.project` file (only M2 has this file). Once this file has been added to the | |||
project it will not be updated again. This way you can easily add your project variables here. |
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.
Avoid usage of personal pronouns:
"This way you can easily add your project variables here." => "This way projects variables can be easily added here."
|
||
# Compression filter. See https://www.varnish-cache.org/trac/wiki/FAQ/Compression | ||
if (req.http.Accept-Encoding) { | ||
if (req.url ~ "\.(jpg|jpeg|png|gif|gz|tgz|bz2|tbz|mp3|ogg|swf|flv)$") { |
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 could add webp in here as well.
set req.url = regsub(req.url, "[?|&]+$", ""); | ||
} | ||
|
||
# Static files caching |
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.
We normally exclude channable as well. See the default varnish templates we use for projects as well.
} | ||
|
||
# Remove all marketing get parameters to minimize the cache objects | ||
if (req.url ~ "(\?|&)(gclid|cx|ie|cof|siteurl|zanpid|origin|fbclid|mc_[a-z]+|utm_[a-z]+|_bta_[a-z]+)=") { |
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.
For Varnish it might be good to besides excluding utm_*, gclid, etc... params to also add gbraids and wbraids to this list: https://blog.wickedreports.com/gclids-gbraids-wbraids-what-is-going-on cc Rutger Rademaker
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.
Can we exclude gbraid and wbraid here as params as well?
@leonhelmus I tested this as you have asked. I found that it works in changing the Software Services but not when changing other variables such as CONFIG__DEFAULT__BUCKAROO_MAGENTO2__ACCOUNT__ACTIVE. I'm not sure if this is the way it's designed to be or not. |
@Epo123 i will probably rework this whole pull request. So this should not be merged yet. |
Added
Changed
nginx-map-dev.conf
,.env.dev.project
do net get overwritten onceit's added to the project for M2.