Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 passingENABLE_SSL
to 0 at container run time?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aENABLE_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.