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

test config before reloading [untested] #569

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

schmunk42
Copy link
Contributor

⚠️ This is an untested change for now ⚠️

Question/discussion:

In general, wouldn't it be better to test the config before reloading nginx.
The expected outcome would be to have nginx still running, with your old config - instead of loosing connection through reverse-proxy to all your sites.

Related to

@schmunk42 schmunk42 changed the title test config before reloading test config before reloading [untested] Sep 14, 2016
@schmunk42
Copy link
Contributor Author

CC: @handcode

@thomasleveil
Copy link
Contributor

thomasleveil commented Feb 21, 2017

even with this, if you restart the nginx-container, nginx will fail to start. This just delays the problem IMHO.

And if you create new containers (using VIRTUAL_HOST) those will be ignored. I'm afraid users will then wonder why their new container is not considered by the nginx-proxy and this will end up in a support request here.

@schmunk42
Copy link
Contributor Author

@thomasleveil Yes that's correct.

But the issue was more like, if there's a broken config somehow and you try to reload, nginx will fail and all your virtual-hosts will be offline, i.e. it's related to #585

I'd say to original behavior might be more desirable in development, while the patch is definitely the way to go in production.

What's about using an ENV variable for this?

@schmunk42
Copy link
Contributor Author

Any news here? Without a fix for this we end up loosing connection to all sites, just because of one broken config/setup.

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.

None yet

3 participants