-
Notifications
You must be signed in to change notification settings - Fork 251
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
fix phoenix/elixir recipe #1252
base: main
Are you sure you want to change the base?
Conversation
postgrex is just the adapter for postgres specifically. there are other ones for different database (mysql, etc.): https://hexdocs.pm/ecto_sql/Ecto.Adapters.SQL.html#module-built-in-adapters
- only copy certain files in each phase so that there isn't a conflict with the lock file - move db migrations to happen at start time rather than bulding - add more ENV defaults like LANG and LANGUAGE
|
||
let mut start_phase = StartPhase::new(start_phase_cmd.to_string()); | ||
// Skip copying any additional files for the start phase | ||
start_phase.only_include_files = Some(vec![]); |
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 don't want to copy any files before starting. if we do, the "lock" file problem occurs.
if mix_exs_content.contains("assets.deploy") { | ||
build_phase.add_cmd("mix assets.deploy".to_string()); | ||
} | ||
|
||
if mix_exs_content.contains("postgrex") && mix_exs_content.contains("ecto") { |
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 won't work because the DATABASE_URL isn't available at this time. Railway's own docs suggesting adding mix ecto.setup
to the start phase instead.
@@ -76,6 +96,8 @@ impl ElixirProvider { | |||
fn default_elixir_environment_variables() -> EnvironmentVariables { | |||
let var_map = vec![ | |||
("MIX_ENV", "prod"), | |||
("LANG", "en_US.UTF-8"), |
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.
LANG and LANGUAGE are required for runtime and good to set as defaults
@@ -148,7 +148,7 @@ impl DockerfileGenerator for BuildPlan { | |||
// Pull the variables in from docker `--build-arg` | |||
variables | |||
.iter() | |||
.map(|var| var.0.to_string()) | |||
.map(|var| format!("{}=\"{}\"", var.0.trim(), var.1.trim())) |
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 line changes the Dockerfile output from:
ARG ELIXIR_ERL_OPTIONS LANG LANGUAGE MIX_ENV NIXPACKS_METADATA
To:
ARG ELIXIR_ERL_OPTIONS="+fnu" LANG="en_US.UTF-8" LANGUAGE="en_US:en" MIX_ENV="prod" NIXPACKS_METADATA="elixir"
Looking at the docker arg docs and from my own testing, this simply sets a default value for the ARG but it can be overridden by setting a variable.
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.
without this line, the rest of the dockerfile doesn't actually inherit values like MIX_ENV for the build phases and then we need to find a different workaround (like prefixing commands with MIX_ENV=prod mix local.rebar --force
)
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 believe we have intentionally not done this because then all variables will be logged in the build logs. The current way of injecting build variables prevents that.
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 injecting default values of environment variables. isn't it okay for these ones to be logged? like MIX_ENV= prod
there shouldn't be any user specific secrets set as defaults afaik.
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.
@coffee-cup is this the only discussion point that's blocking? I will comb through the codebase a bit and see if there are any defaults that we might handle differently. the alternative here is to add some flag to explicitly say "in this case, please fill in the defaults"
start_cmd=start_cmd,} | ||
} else { | ||
// Copy over app files if provided, otherwise copy all files | ||
let copy_cmds = match &self.only_include_files { |
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.
START PHASE: if only_include_files is specified, then use it. Otherwise default to COPY . /app
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 believe only_include_files
was only used when the runtime image was different from the build image. So this makes the uses the same 👍
let mut phase = StartPhase::new("./build/erlang-shipment/entrypoint.sh run"); | ||
phase.only_include_files = Some(vec!["build/erlang-shipment".into()]); | ||
phase | ||
StartPhase::new("./build/erlang-shipment/entrypoint.sh run") |
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.
the phase.only_include_files = Some(vec!["build/erlang-shipment".into()]);
was never working to begin with (since it was fixed in this PR). We can remove the line to keep the previous behaviour.
the only_include_files breaks the build and was never working to begin with
Summary
👋 the FINAL dockerfile output is in the reference section ⬇️
1️⃣ Lock file problem
When deploying a Phoenix Elixir app to Railway there is a conflict with the lock file that cause the error:
lock outdated: the lock is outdated compared to the options in your mix.exs
[1]. This is because the we're copying too many files during multiple phases. The solution is to update the elixir dockerfile builder to only copy specific files.https://elixirforum.com/t/docker-build-started-failing-unchecked-dependencies-for-environment-prod/28844/6
The Railway template for phoenix got around this problem by removing the lock file, which is not behaviour we want.
2️⃣ General updates to the Elixir build
mix ecto.setup
if ecto_sql is detected rather than postgrexecto_sql
is more genericMIX_ENV=prod
Future considerations
The way we run elixir is with
mix phx.server
but actually creating amix release
is the better approach (it creates a binary that can be run). More details here about why release are better. In the future, we should move away frommix phx.server
and use releases instead, but it's beyond the scope of this PR. Here's an example of an Elixir dockerfile that uses releases.3️⃣ Other fixes
only_include_files
in the start phase was ignored. InsteadCOPY . /app
was always added to the generated dockerfileonly_include_files
is set in the start phase, use itARG ELIXIR_ERL_OPTIONS="+fnu" LANG="en_US.UTF-8" LANGUAGE="en_US:en" MIX_ENV="prod" NIXPACKS_METADATA="elixir"
References
[1] Lock file problem
[2] Final Dockerfile
output of
nixpacks build --dockerfile .
on a phoenix elixir project that uses a database: