-
Notifications
You must be signed in to change notification settings - Fork 12
fix: improve dockerfile for staging use #79
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
Conversation
7d9084e to
a17c5d6
Compare
removes DATABASE_URL from docker compose as it was preventing proper running of tests (rspec would connect to dev db when running tests).
a17c5d6 to
ab6f2f1
Compare
| @@ -1,18 +1,26 @@ | |||
| development: | |||
| host: 127.0.0.1 | |||
| default: &default | |||
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.
Pysch issues with upgrade were causing an issue here, which is why the stanza got pulled out.
| <<: *default | ||
| database: awbw_test | ||
|
|
||
| production: |
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.
We're using DATABASE_URL so there's no reason to set these except for configuration like pool size
| # Only run if MySQL is the DB | ||
| if adapter.include?('mysql') | ||
| sql_file = Rails.root.join('db', 'awbw_dml_only.sql') | ||
| sql_file = Rails.root.join('db', 'awbw_dml_only.sql') |
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 is going to fall out in the rebase from other PRs but solid catch,
| build: | ||
| context: . | ||
| dockerfile: Dockerfile.dev # <— use the dev Dockerfile | ||
| dockerfile: Dockerfile |
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.
We're only using compose locally right? Or are you trying to get everything together in one place?
| RUN apt-get update -qq && apt-get install -y \ | ||
| # Make sure RUBY_VERSION matches the Ruby version in .ruby-version | ||
| ARG RUBY_VERSION=3.3.8 | ||
| FROM docker.io/library/ruby:$RUBY_VERSION-slim AS base |
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.
Consider locking this to the debian version we're shipping. Otherwise you can end up surprised when the rails docker team switched the base image underneath you 😄
|
I think we maybe should close this? |
removes DATABASE_URL from docker compose as it was preventing proper
running of tests (rspec would connect to dev db when running tests).
this also swaps to the same image the default rails new generates.