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: docker #254

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

chore: docker #254

wants to merge 8 commits into from

Conversation

DanielGluskin
Copy link
Collaborator

@DanielGluskin DanielGluskin commented Dec 17, 2022

  • A dockerfile according to official Turborepo docs
  • Change all internal deps to'*' instead to a specific version
  • Remove all package-lock files, except for root

This PR doesn't handle production build - dev dependencies stay inside the docker, also source TS code.

(Also, need to review devDeps for all libraries, I believe some are wrong).

@codecov
Copy link

codecov bot commented Dec 17, 2022

Codecov Report

Base: 90.92% // Head: 90.92% // No change to project coverage 👍

Coverage data is based on head (75154d8) compared to base (487b4a1).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #254   +/-   ##
=======================================
  Coverage   90.92%   90.92%           
=======================================
  Files          16       16           
  Lines         595      595           
  Branches       43       43           
=======================================
  Hits          541      541           
  Misses         53       53           
  Partials        1        1           
Flag Coverage Δ
app 91.97% <ø> (ø)
generator 63.63% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rluvaton
Copy link
Collaborator

Great job!

  • Change all internal deps to'*' instead to a specific version

For large applications I think we should not do that as you lose the advantage of different services moving at a different pace and every change even if not related to production code (e.g updating some dev dependency with breaking change) will force you to change everywhere

  • Remove all package-lock files, except for root

Care to explain why?

COPY package-lock.json /app/
COPY package-lock.json /app/
RUN npm ci --only=production
RUN npm install turbo -g
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can potentially install a version that is different from the version in the package.json and may work differently if it has some breaking changes

FYI, if it has cache it will not install the turbo package even if there is some security fix for example

The 2nd point is ok as when deploying we should not have docker cache (90% sure for Github Actions)

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.

2 participants