Skip to content
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

Added configuration of custom fastcgi_params [fixes #389] #396

Merged
merged 5 commits into from
Aug 13, 2014

Conversation

chaosmail
Copy link
Contributor

I created a PR for Fastcgi Params #389 and proposed the key "params" for adding custom fastcgi_params in the puppet configuration.

nginx::resource::location { "root":
    ...
    ensure          => present,
    fastcgi         => "127.0.0.1:9000",
    params          => {
        'APP_ENV' => 'local',
    },
}

@3flex
Copy link
Contributor

3flex commented Aug 11, 2014

Hi @chaosmail, couple of comments:

  • The parameters should be something more explicit than params, since there could also be a uswgi_params and scgi_params added in future. I'd suggest fastcgi_param - though this is very close to fastcgi_params which would be a preferable name it's already in use.
  • I suggest adding the nginx directives to the fastcgi.erb template instead of fastcgi_params.erb, so people can use both the fastcgi_params template as well as custom directives at the same time, or use the standard fastcgi_params for some locations and custom params for other locations.
  • Return a deprecation notice when fastcgi_script != undef, since this new parameter makes that one redundant.

@chaosmail
Copy link
Contributor Author

Hi @3flex,

I followed all your comments in the latest commit, the configuration now looks like this

nginx::resource::location { "root":
    ...
    ensure          => present,
    fastcgi         => "127.0.0.1:9000",
    fastcgi_param   => {
        'APP_ENV' => 'local',
    },
}

@3flex
Copy link
Contributor

3flex commented Aug 13, 2014

Awesome, that looks great to me. It's a pity that the parameter can't be called "fastcgi_params" but that might be a future enhancement. The functionality itself will certainly be useful.

@3flex
Copy link
Contributor

3flex commented Aug 13, 2014

Just one last thing - if you add "fixes #389" to the issue description or one of the commits it will automatically close that issue when this is merged.

@chaosmail chaosmail changed the title Added configuration of custom fastcgi_params Added configuration of custom fastcgi_params [fixes #389] Aug 13, 2014
@chaosmail
Copy link
Contributor Author

I added it in the description, hope that's ok

@jfryman
Copy link
Contributor

jfryman commented Aug 13, 2014

👍

jfryman pushed a commit that referenced this pull request Aug 13, 2014
Added configuration of custom fastcgi_params [fixes #389]
@jfryman jfryman merged commit d0f1f85 into voxpupuli:master Aug 13, 2014
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Oct 23, 2017
Added configuration of custom fastcgi_params [fixes voxpupuli#389]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants