Reorganize templates for clearer understanding #970
Conversation
|
I think we were talking about this online, and this is roughly the direction I was hoping this part would head in. I'd like some input from @bastelfreak or @3flex, but I think it might make more sense to use concat() for some or all of the template joining (as is done in most of the rest of the module) vs. including templates from within another template, but curious to see what other folks think. Obviously, quite a few tests will need to be reworked to get this going. Note that the server -> vhost change is in progress; will probably need to rebase after that gets merged. |
|
Big fan of concat. |
|
@jyaworski I too am a fan of concat, but I don't think it applies here. The location blocks may be of arbitrary count, and with arbitrary options within each block. To use concat, the order would be a nightmare, and perhaps simply not possible. @wyardley I thought I'd toss up the PR to generate discussion. I've not looked at the tests yet, but I think of we can agree about a direction here, that would certainly make this module easier to work with. I care about locations at the moment, but this pattern could be extended elsewhere. |
|
I've only skimmed over this, but I'm 100% in support of this change, it's long overdue. I agree that concat isn't the right choice here for the reasons @xaque208 outlined above. There are very few order-dependent directives that can be set within a location block (I think just directives from the rewrite and access modules?) so concat would add a lot of complexity for little gain. Better to treat those two module's directives specially instead. Using conditionals like in the PR is a better way to go IMHO. |
Without this change, the structure of the templates for locations are bit rigid and hard to understand. Each component of a location uses an isolated template which means that much of the common logic is hard coded to a particular location style, the deployment of which is chosen based on a somewhat arbitrary idea of what it means to be a location, and in some cases, the module gets it wrong. In cases where there is seeming correctness, modifications to a particular selection of logic are duplicated among the nested templates. This work is the results of what was necessary for me to understand what the templates were doing, and to deploy a fastcgi PHP application. As such, the templates have been centralized and conditions about their functionality have been moved into the template to determine if rendering needs to be taken. This allows a more complete, and leaves potential for more complex examples easier to understand and reason about. Also here is the addition of a new param fastcgi_index on the location class. This work should result in on-disk configurations that are functionally equivalent to what was in place before, except in places where the module was masking conflicting options that were not being rendered in the templates.
|
I've updated the primary template and the tests pass. Each template render call now is within its own |
|
Thank you. |
|
Was hoping to get #968 merged before we merged anything else, but no worries, we can re-do it this week probably. |
| @@ -1,3 +1,4 @@ | |||
| <% if @proxy -%> | |||
| proxy_pass <%= @proxy %>; | |||
| proxy_read_timeout <%= @proxy_read_timeout %>; | |||
| proxy_connect_timeout <%= @proxy_connect_timeout %>; | |||
There was a problem hiding this comment.
Is there a reason why proxy_send_timeout is not part of this if statement?
There was a problem hiding this comment.
@ubellavance Only oversight. Feel free to send a PR for any change that needs to be made here.
There was a problem hiding this comment.
@ubellavance
This PR is already merged, so no point adding a review / feedback here.
I don't think the issue is that proxy_send_timeout isn't part of the template; it's not a parameter for the location resource at all. As things in the past have tended to be added piecemeal as people needed them, there's not a lot of consistency in terms of which supported parameters are allowed in which contexts. As best I can tell, proxy_send_timeout isn't currently directly supported as a parameter for server blocks either; it's just supported at the top level nginx config. This could be because most people don't need to set different values for this for different sites, and / or that the people who do just use the location_cfg_append or similar directives.
That said, it's not a big limitation usually, because you can generally use the various custom directives to add those parameters if they're needed in server or location contexts. As @xaque208 says, PRs (with tests) are welcome.
There was a problem hiding this comment.
No problem. I saw that yesterday while browsing the code to make changes in my environment. I think I'll pass for the PR this time (mostly because I don't have much time, but also because this module is a bit too complex for me) and I realize that I'm already using the parameter in the global directive.
I must say, though, that this module rocks and it's very nice to see how many people invest that much time into it.
Without this change, the structure of the templates for locations are bit rigid and hard to understand. Each component of a location uses an isolated template which means that much of the common logic is hard coded to a particular location style, the deployment of which is chosen based on a somewhat arbitrary idea of what it means to be a location, and in some cases, the module gets it wrong. In cases where there is seeming correctness, modifications to a particular selection of logic are duplicated among the nested templates. This work is the results of what was necessary for me to understand what the templates were doing, and to deploy a fastcgi PHP application. As such, the templates have been centralized and conditions about their functionality have been moved into the template to determine if rendering needs to be taken. This allows a more complete, and leaves potential for more complex examples easier to understand and reason about. Also here is the addition of a new param fastcgi_index on the location class. This work should result in on-disk configurations that are functionally equivalent to what was in place before, except in places where the module was masking conflicting options that were not being rendered in the templates.
Without this change, the structure of the templates for locations are bit rigid and hard to understand. Each component of a location uses an isolated template which means that much of the common logic is hard coded to a particular location style, the deployment of which is chosen based on a somewhat arbitrary idea of what it means to be a location, and in some cases, the module gets it wrong. In cases where there is seeming correctness, modifications to a particular selection of logic are duplicated among the nested templates. This work is the results of what was necessary for me to understand what the templates were doing, and to deploy a fastcgi PHP application. As such, the templates have been centralized and conditions about their functionality have been moved into the template to determine if rendering needs to be taken. This allows a more complete, and leaves potential for more complex examples easier to understand and reason about. Also here is the addition of a new param fastcgi_index on the location class. This work should result in on-disk configurations that are functionally equivalent to what was in place before, except in places where the module was masking conflicting options that were not being rendered in the templates.
Without this change, the structure of the templates for locations are
bit rigid and hard to understand. Each component of a location uses an
isolated template which means that much of the common logic is hard
coded to a particular location style, the deployment of which is chosen
based on a somewhat arbitrary idea of what it means to be a location,
and in some cases, the module gets it wrong. In cases where there is
seeming correctness, modifications to a particular selection of logic
are duplicated among the nested templates.
This work is the results of what was necessary for me to understand what
the templates were doing, and to deploy a fastcgi PHP application. As
such, the templates have been centralized and conditions about their
functionality have been moved into the template to determine if
rendering needs to be taken. This allows a more complete, and leaves
potential for more complex examples easier to understand and reason
about.
Also here is the addition of a new param fastcgi_index on the location
class.
This work should result in on-disk configurations that are functionally
equivalent to what was in place before, except in places where the
module was masking conflicting options that were not being rendered in
the templates.