-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Created docker-compose configuration and tested in local to run as container #416
Conversation
Signed-off-by: Jinna Baalu <[email protected]>
Signed-off-by: Jinna Baalu <[email protected]>
…pose for the same
Wouldn't it be better to use a |
That is individual choice, rather complicating the compose, i want to keep it plain, when you run in your environment it should be based on their thought to apply. |
@@ -43,7 +43,7 @@ | |||
# config.assume_ssl = true | |||
|
|||
# Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies. | |||
config.force_ssl = true | |||
config.force_ssl = ENV.fetch("SSL_ENABLE", "true") == "true" |
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 would suggest using either SSL_ENABLED
or ENABLE_SSL
and keeping it consistent for the upcoming env vars
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.
You want me to have the variable ENABLE_SSL
confused. Please let me know I can update
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.
ENABLE_SSL
or SSL_ENABLED
=> OK
SSL_ENABLE
=> NOT OK
@@ -43,7 +43,7 @@ | |||
# config.assume_ssl = true | |||
|
|||
# Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies. | |||
config.force_ssl = true | |||
config.force_ssl = ENV.fetch("SSL_ENABLE", "true") == "true" |
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 possible, can we stick with this solution?
ports: ["5432:5432"] | ||
|
||
maybe: | ||
image: ghcr.io/maybe-finance/maybe:latest |
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 docker-compose
file will be used in development, so I'm not sure if we want to reference our official image as it will be optimized for production. Was that the intention here?
|
||
- name: Push Docker image to GHCR | ||
run: | | ||
docker push ghcr.io/${{ github.repository }}/maybe:latest |
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.
Does the Dockerfile
that we're pushing here build for you locally? I think we may be a bit early to start pushing to ghcr as we have a lot of services to still add.
on: | ||
push: | ||
branches: | ||
- main |
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.
Wouldn't it suffice to only build and push on creating tags?
Closing this out for now. I think these changes belong in a dedicated PR to build out a Dockerfile, address SSL environment variables, and an automated push flow for the Dockerfile. This is all being tracked in #295 |
No description provided.