fly-apps / dockerfile-rails

Provides a Rails generator to produce Dockerfiles and related files.
MIT License
489 stars 38 forks source link

Missing RUN statement when using execjs and Yarn #81

Closed jeff-french closed 10 months ago

jeff-french commented 10 months ago

When generating a Dockerfile for a project that uses execjs and Yarn, the "Install yarn" section of the generated Dockerfile is missing a RUN instruction.

Steps to reproduce

The steps below were used to recreate the issue and the result is available here: https://github.com/jeff-french/dockerfile-rails-issue-reproduction

# Check environment
ruby --version
# ruby 3.2.2 (2023-03-30 revision e51014f9c0) [arm64-darwin22]
rails --version
# Rails 7.1.3
node --version
# v20.11.0
yarn --version
# 4.0.2

# New rails project
rails new blog
cd blog

# Setup Yarn
corepack enable
yarn init -2
yarn set version stable
yarn install

# Add execjs
bundle add execjs

# dockerfile-rails
bundle add dockerfile-rails --optimistic --group development
rails g dockerfile --force

Actual output

This will produce an invalid section starting around line 40 of the generated Dockerfile

# Install yarn
ARG YARN_VERSION=4.0.2
    corepack enable && \
    corepack prepare yarn@$YARN_VERSION --activate

Expected output

# Install yarn
ARG YARN_VERSION=4.0.2
RUN corepack enable && \
    corepack prepare yarn@$YARN_VERSION --activate

Investigation

The issue appears to be caused when the install_node template is called the second time to setup Yarn: https://github.com/fly-apps/dockerfile-rails/blob/ec2b18e331ebbdfa1e5331268ba3b7f64b66ccc0/lib/generators/templates/Dockerfile.erb#L67-L70

In this call node_version gets set to nil due to the usage of execjs and yarn_version get set to 4.0.2.

When the install_node template gets rendered from this call, the following section gets skipped entirely since node_version=nil: https://github.com/fly-apps/dockerfile-rails/blob/ec2b18e331ebbdfa1e5331268ba3b7f64b66ccc0/lib/generators/templates/_install_node.erb#L23-L34

and then this section attempts to append to the RUN command from the previously skipped section: https://github.com/fly-apps/dockerfile-rails/blob/ec2b18e331ebbdfa1e5331268ba3b7f64b66ccc0/lib/generators/templates/_install_node.erb#L35-L46

Solution

I was able to get the expected output by changing the following section: https://github.com/fly-apps/dockerfile-rails/blob/ec2b18e331ebbdfa1e5331268ba3b7f64b66ccc0/lib/generators/templates/_install_node.erb#L41-L43

to

<% else -%>
<% if node_version -%>   <% else %>RUN<% end %> corepack enable && \
<% end -%>

Caveats

This fix worked in this particular situation but I'm not sure if it is a drop in solution when execjs is not being used and the node_install ends up rendering the full template.

rubys commented 10 months ago

Doesn't break any existing test, so I merged it. Arguably deserves its own test, but I'll leave that for later.

Thanks!