-
Notifications
You must be signed in to change notification settings - Fork 848
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
Support multiple php versions through vvv-config.yml & utilities #1055
Support multiple php versions through vvv-config.yml & utilities #1055
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.
This is pretty slick! I left a couple comments inline. We should also update at least the wordpress-develop
repository with the new nginx template variables, even if they end up being the default settings. This should help as a guide.
@@ -48,6 +48,7 @@ vvv_config['sites'].each do |site, args| | |||
defaults['branch'] = 'master' | |||
defaults['skip_provisioning'] = false | |||
defaults['allow_customfile'] = false | |||
defaults['upstream'] = 'php' |
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.
How about nginx_upstream
or phpfpm_upstream
? I doubt we'll ever use "upstream" elsewhere, but it may make it more clear.
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.
I decided on upstream
instead of php_version
as i realised it could be used for so much more than PHP, just as a test i have already run it with Node & Python upstreams.
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.
Good point, using php
here doesn't make sense.
I'm still going to be a pain. Let's do nginx_upstream
. I think that clarifies the intent of the config. 😄
} | ||
|
||
include /etc/nginx/upstreams/*.conf; |
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.
Looks like spacing may be off just a bit in these lines.
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.
Fixed, my IDE had used tabs instead of spaces 🙄
No reason to keep this waiting. I'll have time to do some more testing this weekend, but I'd like to get this in earlier than later. Thanks @LoreleiAurora! |
This adds an additional optional variable of
upstream
to vvv-config.yml to allow you to set the nginx upstream.It also switches xDebug from source to the PPA as this is compatible with multiple PHP versions.
An example config for testing:
Requires Varying-Vagrant-Vagrants/vvv-utilities#3 to stop my custom repo being needed.