FIX: default enable_ssl to be 1 #985
Conversation
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: |
|
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/connection-refused-on-self-hosted-instance-after-rebuild/376224/4 |
| env: | ||
| ENABLE_SSL: 1 |
There was a problem hiding this comment.
I don't quite understand why need to include this ENV flag to enable SSL when the fact that this template is included means SSL should be enabled.
There was a problem hiding this comment.
I'm attempting to break that assumption a bit and allow it to be configurable at runtime
IE go from: "including this template enables ssl unconditionally"
to: "including this template allows ssl to be configured optionally"
This makes it easier for anyone to build and ship pre-bootstrapped images that can be configured at runtime, with no launcher required.
There was a problem hiding this comment.
to: "including this template allows ssl to be configured optionally"
But this is only true if the template doesn't set ENABLE_SSL for the user. With this change, it seems like we are trying to support the case where a user includes the SSL templates but wants to disable it by passing ENABLE_SSL to 0 at container run time?
There was a problem hiding this comment.
This makes it easier for anyone to build and ship pre-bootstrapped images that can be configured at runtime, with no launcher required.
If this is the goal, perhaps we can specify a directory which we will look into for SSL certificates in the install-ssl script? If the certificates are present, do the necessary configuration update to nginx. I kind of feel like a ENABLE_SSL knob is unnecessary when we can already make a decision based on the presence of SSL certificates.
There was a problem hiding this comment.
This particular change was an attempt at mirroring what I did for letsencrypt (via LETSENCRYPT_ACCOUNT_EMAIL), but detecting the presence of an ssl cert makes sense as a better knob to me, I'm game with working in that direction. 👍
Merging this to resolve the immediate issue in the meantime.
Default ENABLE_SSL to 1 when template is included.