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

Docker image: use package-lock.json and only include relevant parts #241

Merged
merged 3 commits into from
Aug 25, 2020

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented May 29, 2020

Together with the TravisCI Job definition added in #239 / #240 and configured to run in TravisCI settings, I suggest this PR replace #228.

Closes #228.

@consideRatio consideRatio force-pushed the docker-image-from-package-lock branch from f3ed79d to b374681 Compare May 29, 2020 13:36
@consideRatio consideRatio marked this pull request as draft May 29, 2020 13:45
@consideRatio consideRatio force-pushed the docker-image-from-package-lock branch from b374681 to 8733600 Compare May 29, 2020 13:56
@consideRatio consideRatio requested a review from minrk May 29, 2020 14:04
@consideRatio consideRatio marked this pull request as ready for review May 29, 2020 14:04
@@ -41,4 +41,4 @@ case "$@" in
ARGS="--api-ip=0.0.0.0";;
esac

exec configurable-http-proxy $ARGS "$@"
exec bin/configurable-http-proxy $ARGS "$@"
Copy link
Member Author

Choose a reason for hiding this comment

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

Explanation of line

Due to the change from npm install -g to npm ci where the -g flag isn't available, we need to reference the local executable instead.

I tried copying this to /usr/local/bin but then I got the following error, so I think this is a good solution.

/srv/configurable-http-proxy $ ./chp-docker-entrypoint 
internal/modules/cjs/loader.js:969
  throw err;
  ^

Error: Cannot find module '../package.json'
Require stack:
- /usr/local/bin/configurable-http-proxy
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:966:15)
    at Function.Module._load (internal/modules/cjs/loader.js:842:27)
    at Module.require (internal/modules/cjs/loader.js:1026:19)
    at require (internal/modules/cjs/helpers.js:72:18)
    at Object.<anonymous> (/usr/local/bin/configurable-http-proxy:11:9)
    at Module._compile (internal/modules/cjs/loader.js:1138:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1158:10)
    at Module.load (internal/modules/cjs/loader.js:986:32)
    at Function.Module._load (internal/modules/cjs/loader.js:879:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/usr/local/bin/configurable-http-proxy' ]
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm while it works, because the current working directory is the same as this files location, but I think it is a bit unrobust. So, I'll add the folder to path instead from the Dockerfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the Dockerfile's PATH env var to find the ./bin folder

!chp-docker-entrypoint
!index.js
!package*.json
!LICENSE
Copy link
Member Author

@consideRatio consideRatio May 29, 2020

Choose a reason for hiding this comment

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

Explanation of adding .dockerignore

I added this file because I noted that we copied things from node_packages and felt that it could risk causing some issue, and that it would trigger rebuilds for no use potentially.

I got a warning about the node_packages being copied and existing, this is what led me to this change. It will come with the drawback of needing to update this as well if we add something to include, but hopefully we will add that within the lib folder.

Copy link
Member

Choose a reason for hiding this comment

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

I think instead of setting what gets added in dockerignore, we should whitelist in COPY directives instead. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I did that first, and then switched to this option for aesthetics.

When I concluded it was required to look something like below, with three lines instead of one, I felt better about a .dockerignore and COPY . /dest than multiple COPY statements - one for each folder and one of the files. It turned out that one cannot copy an entire folder next to files files in a COPY statement, then the contents of the folder is copied to the destination rather than the entire folder.

# the option to a .dockerignore file afaik
COPY our-bin-folder /destination
COPY our-lib-folder /destination
COPY list-of-individual-files /destination

Let me know what you think, then I'll go with that!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ping @minrk

Copy link
Member Author

@consideRatio consideRatio Jul 4, 2020

Choose a reason for hiding this comment

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

I added a comment about the .dockerignore file next to the COPY . statement the risk of missing out on what goes on is minimized.

@consideRatio consideRatio marked this pull request as ready for review May 29, 2020 14:44
@minrk minrk merged commit 562b253 into jupyterhub:master Aug 25, 2020
@minrk
Copy link
Member

minrk commented Aug 25, 2020

Thanks! I think we can reach a better compromise in .dockerignore, but this is still an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants