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

Adding some params to the uwsgi template that I needed. And some formating. #855

Closed
wants to merge 1 commit into from

Conversation

jostmart
Copy link

@jostmart jostmart commented Aug 18, 2016

Added some params to the uwsgi template that I needed. And some simple formating.

@bastelfreak
Copy link
Member

Hi @jostmart, could you add tests to the changes?

@ffrank
Copy link

ffrank commented Sep 8, 2016

Is this safe? Can we be certain that this has no negative impact for existing setups?

@jyaworski
Copy link
Member

I don't see any problem with this. It looks like it's just adding:

+uwsgi_param REQUEST_SCHEME  $scheme;
+uwsgi_param HTTPS           $https if_not_empty;

@ffrank
Copy link

ffrank commented Sep 12, 2016

Yes. Is it conceivable that a module user does specifically not want either of those?

@wyardley
Copy link
Collaborator

wyardley commented Oct 8, 2016

@jostmart: will you be able to add tests? If there's no more activity in about a month, this will be closed. Would the fix in #767 work instead? It's generic, but allows adding arbitrary uwsgi_params, which might be more flexible.

I don't deal with nginx / uwsgi together much, but I agree with @ffrank that it might be better not to default to adding them if they're not explicitly set.

@wyardley
Copy link
Collaborator

wyardley commented Nov 5, 2016

@jostmart I think in principle this is fine to add; are you able to add tests?

@wyardley
Copy link
Collaborator

wyardley commented Feb 2, 2017

I'm not even sure that tests are needed here, now that I think about it, but the rename is causing a conflict.

@ffrank
Copy link

ffrank commented Feb 13, 2017

Well, to make this easier: The three added parameters take very sensible values, so I think it should be fine to just merge this.
@jostmart will you be able to take care of those merge conflicts? I'm in favor of just merging once that is done.

@wyardley
Copy link
Collaborator

@ffrank agreed, but so far, no response (I'm guessing it just needs to be moved to templates/server?

Does someone want to take a stab at reworking it and re-submitting (with attribution to the original author)?

@jostmart
Copy link
Author

jostmart commented Mar 7, 2017

Hi I will try to look at it in a couple of days. The change I made have been working great for me since I added it.

@vinzent
Copy link
Contributor

vinzent commented Apr 11, 2017

@jostmart any chance to look into rebasing?

@wyardley
Copy link
Collaborator

If you don't mind, can we pick this up in #1076?

@wyardley
Copy link
Collaborator

closing via #1076

@wyardley wyardley closed this Apr 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants