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

dockerfile: no longer require package-lock #29

Merged
merged 3 commits into from Mar 21, 2023

Conversation

BethGriggs
Copy link
Collaborator

npm ci requires package-lock.json, so if a sample application being imported doesn’t have it then build is going to fail.

We want the DevFile to work for as many applications as possible out of the box, so this adds a fallback for the case a project does not have a package-lock.json

Comment on lines +4 to +5
# Copy package.json, and optionally package-lock.json if it exists
COPY package.json package-lock.json* ./
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The final npm start command in the Dockerfile relies on a package.json existing. This copy command means we fail early if it doesn't.

Choose a reason for hiding this comment

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

I assume the * at the end is what gets us the does not fail if package-lock.json does not exist but it would copy over package-lock.json2 etc. as well. We might be able to tighten that up but I don't think that is a blocker for this PR.

Comment on lines +8 to +11
RUN \
if [ -f package-lock.json ]; then npm ci; \
else npm install; \
fi
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fallback in case of no package-lock.json. It's unusual for a Node.js project repository to not have one these days, but let's cater for the exception.

@BethGriggs BethGriggs marked this pull request as ready for review March 21, 2023 18:51
Copy link

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@BethGriggs BethGriggs merged commit 5c6e713 into nodeshift-starters:main Mar 21, 2023
1 check passed
@BethGriggs BethGriggs deleted the dockerfile branch March 21, 2023 21:45
Roming22 added a commit to Roming22/devfile-sample-nodejs that referenced this pull request Apr 10, 2023
dockerfile: no longer require package-lock (nodeshift-starters#29)
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

4 participants