Skip to content
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

Update web dockerfile to use dev deps when building prod assets #985

Merged
merged 2 commits into from
May 23, 2018

Conversation

klingerf
Copy link
Member

This branch fixes a master branch CI failure that was introduced in #976. Specifically, the bin/docker-build-web script is failing due to docker not being able to find dev dependencies, such as "babel-loader", when running NODE_ENV=production yarn webpack.

The issue arises from these stages in the Dockerfile:

# node dependencies
RUN $ROOT/bin/web setup --pure-lockfile

# frontend assets
# set the env to production *after* yarn has done an install, to make sure all
# libraries required for building are included.
ENV NODE_ENV production
RUN $ROOT/bin/web build

Calling web setup the first time effectively runs NODE_ENV=development yarn, which installs the development dependencies that we need. Unfortunately, calling web build also calls web setup, and that causes us to re-run the setup with NODE_ENV=production yarn, which removes the development dependencies that were previously installed.

A different fix for this would be to modify web build to stop calling setup, and require that folks run the setup command explicitly, as needed. Since setup only really needs to be called when package.json changes, that would also speed up steady-state development. @grampelberg thoughts?

@klingerf
Copy link
Member Author

A different fix for this would be to modify web build to stop calling setup, and require that folks run the setup command explicitly, as needed.

I have this change ready to go and am going to push it as a separate commit to this branch.

Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the full context on this change and #976, but can confirm that bin/docker-build, bin/web, and bin/web run work on this branch.

Copy link
Contributor

@grampelberg grampelberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gah, totally my bad =/ Sorry about that.

Signed-off-by: Kevin Lingerfelt <kl@buoyant.io>
Signed-off-by: Kevin Lingerfelt <kl@buoyant.io>
@klingerf klingerf merged commit cb24154 into master May 23, 2018
@klingerf klingerf deleted the kl/fix-web branch May 23, 2018 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants