Rework passenger VHost and Directories #1778
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Looks fine to me but would like a +1 from someone else |
david22swan
approved these changes
Apr 18, 2018
|
I can't speak to the code quality, but I could really use this capability right now. |
|
@hunner Could you give this a quick look please? |
|
Any chance of this getting merged? |
|
Hey @smortex , I'd be keen to get your changes in, would you have any time to rebase this for us then I can re-review for merge? Thanks! |
While here, ensure we have a line-break after PassengerStartupFile.
Follow the Passenger's documentation ordering for parameters: https://www.phusionpassenger.com/library/config/apache/reference No functional change.
Without this change, explicitely setting these parameters to 'false' does the same as not setting the parameters at all. Since both parameters default to 'Off', this has no direct consequence, but since we will introduce boolean that defaults to 'On', always output the parameters with their value when they are set.
RailsBaseURI and RackBaseURI where deprecated in Passenger 3.0.0. PassengerBaseURI should be used instead. For some reason, the implementation expected an array, but setting this parameter with multiple values in a single context does not really make sense.
A bunch of parameters can be set on a virtual host, but also directories. This allows to build complex setups composed of multiple applications available through a single VirtualHost.
These parameters used to be String (not enforced by types). In order to avoid changing the Unit Tests, they were kept as-is. Now that refactoring is done, switch to a better type.
smortex
force-pushed
the
rework-passenger-vhost-and-directories
branch
from
bed0677
to
f55a61c
Compare
|
Hi @HelenCampbell! I just rebased the branch on top of master. Thanks! |
|
@smortex Reviewed and ran this PR through an adhoc jenkins pipeline with no issues. I'm happy with the changes so I'll get this merged. Thank you for all the work you put into this! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull-Request allow setting all supported parameters to VirtualHosts and Directories.
It also fixes a few things and has a few enhancements:
The combined commits is a pain to read, but hopefully reviewing commits one by one should be easier :-)