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

chore: dockerize development environment #1308

Merged
merged 6 commits into from
Oct 12, 2021
Merged

chore: dockerize development environment #1308

merged 6 commits into from
Oct 12, 2021

Conversation

millotp
Copy link
Contributor

@millotp millotp commented Oct 5, 2021

Ticket: DI-14

In the sake of harmonisation and to ease development, this PR adds a docker file in which you can run the test or lint the code, while using your normal IDE.

They are a few caveats, the first one is that because we mount the project to the /app folder, the node_modules are shared and slow down the execution by a lot. To avoid that, the node_modules are installed in the parent folder (the root) and an empty volumes is mounted to avoid copying the pacakges from the host.
This works fine but prevent the modification of the packages, and the image must be rebuilt when some are added.

I used Docker Desktop in the readme to install docker because docker machine is deprecated and the code is archived.

To test the Dockerfile, follow the DOCKER_README.MD instructions and then run yarn test:unit for example.

@millotp millotp added the chore label Oct 5, 2021
@millotp millotp self-assigned this Oct 5, 2021
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 5, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2da620a:

Sandbox Source
javascript-client-app Configuration

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

👌

WORKDIR /

# Needed to compile some npm packages
RUN apk add --no-cache bash python3 make g++
Copy link
Contributor

Choose a reason for hiding this comment

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

which packages is this needed for? could we avoid them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

python3, make, and g++ are required for iltorb, a dependency of bundlesize and rollup-plugin-filesize, so it's dev only but the docker environment is meant for dev.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, could we maybe have a look at updating those versions of bundlesize & rollup-plugin-filesize? if I remember it correctly in their later versions they rely on the node builtin brotli support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking deeper into it only bundlesize is requiring iltorb, but it doesn't seem to be maintained anymore, there is a PR opened for 6 months, and another package that solves that and doesn't require any node-gyp.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's leave it as-is then for now @millotp :)

Dockerfile Outdated Show resolved Hide resolved
DOCKER_README.MD Outdated Show resolved Hide resolved
DOCKER_README.MD Show resolved Hide resolved
Dockerfile Outdated
@@ -1,5 +1,5 @@
# Dockerfile
FROM node:10-alpine
FROM node:12.16.2-alpine
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be read from the nvmrc directly to not have a duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right it's totally possible, once renovate bot will be installed it won't be an issue but it's a good idea for now.

Haroenv
Haroenv previously approved these changes Oct 5, 2021
Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

good to go

Copy link

@DevinCodes DevinCodes left a comment

Choose a reason for hiding this comment

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

This is great, thank you @millotp. It's almost good to go! 🚀

Dockerfile Outdated
# Install the dependencies in the parent folder so they don't get overriden by the bind mount
WORKDIR /

# Needed to compile some npm packages

Choose a reason for hiding this comment

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

Suggested change
# Needed to compile some npm packages
# We need to install some dependencies for bundlesize (https://github.com/siddharthkp/bundlesize/pull/370)

DOCKER_README.MD Outdated
--env ALGOLIA_ADMIN_KEY_1 \
--env ALGOLIA_SEARCH_KEY_1 \
--env ALGOLIA_APPLICATION_ID_2 \
--env ALGOLIA_ADMIN_KEY_2

Choose a reason for hiding this comment

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

Suggested change
--env ALGOLIA_ADMIN_KEY_2
--env ALGOLIA_ADMIN_KEY_2 \

@millotp
Copy link
Contributor Author

millotp commented Oct 5, 2021

Thanks @Haroenv and @DevinCodes for the review !

@Haroenv
Copy link
Contributor

Haroenv commented Oct 6, 2021

The test is failing because the node docker container of circleCI that doesn't have the latest node versions is bundled with an outdated ssl implementation that resolves lets encrypt wrong, and doesn't allow yarn installs

@DevinCodes
Copy link

@Haroenv is there anything that we can do to resolve that? Or do we depend on CircleCI to resolve this in their image?

@shortcuts
Copy link
Member

I thought the fixes ... only worked with issues, sorry

@shortcuts shortcuts reopened this Oct 8, 2021
CONTRIBUTING.md Outdated Show resolved Hide resolved
@Haroenv
Copy link
Contributor

Haroenv commented Oct 8, 2021

What do you think of harmonising docker image in CI and this docker image in some later step?

@millotp
Copy link
Contributor Author

millotp commented Oct 12, 2021

It might be possible to use this image in the CI yes, I need to check what the circle ci image does specifically, it might add some mandatory stuff.

@millotp millotp merged commit 7c38e64 into master Oct 12, 2021
@DevinCodes DevinCodes deleted the chore/dockerize branch October 12, 2021 08:22
@millotp millotp mentioned this pull request Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants