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

socket.io prefix is not required in nginx config #1434

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

neerajshukla1911
Copy link

socket.io prefix is not required

@neerajshukla1911 neerajshukla1911 changed the title socket.io prefix is not required socket.io prefix is not required in nginx config Feb 3, 2017
@lukeyeager
Copy link
Member

I just copied that from the Flask-SocketIO documentation:
https://flask-socketio.readthedocs.io/en/latest/#using-nginx-as-a-websocket-reverse-proxy

Are you seeing any problems?

@neerajshukla1911
Copy link
Author

With current master branch code it works fine. But if digits is run with url-prefix (#1397) code then it throws error since its not correct url. Removing socket.io prefix will make it work in both cases.

@lukeyeager
Copy link
Member

Yeah but if you're using #1397 then you're probably going to be changing several other things in your nginx config, right?

However, I'm not opposed to merging this if it doesn't break anything for the standard case. When I get back in on Monday I'll test it out.

BTW, I'll need you to send a signed CLA before I can accept this contribution.
https://github.com/NVIDIA/DIGITS/blob/digits-5.0/.github/CONTRIBUTING.md#pull-requests

@neerajshukla1911
Copy link
Author

Change in this pull request also makes same proxy_pass for both location '/' and '/socket.io'. So further redundancy can be removed using upstream directive like below
upstream digits_server {
server localhost:34448;
}
This will make config clean. Should I make upstream changes in this pull request only ?
I will send signed CLA today.

@lukeyeager
Copy link
Member

Why don't you update the PR to show me what you mean. I'm still not convinced anything of the changes you're proposing would change any behavior (unless possibly paired with #1397).

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.

2 participants