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

Add dockerFile option #128

Merged
merged 4 commits into from
Jan 18, 2018
Merged

Conversation

cgrimal
Copy link

@cgrimal cgrimal commented Jan 17, 2018

Define the dockerFile option that is mutually exclusive with the dockerImage option.

If dockerFile is provided, it must be the name of the Dockerfile file, placed in the current directory.
When building requirements, a Docker image is built based on the provided Dockerfile, and requirements are then built using this image.

Limitations:

  • built every time (even if image exists and Dockerfile has not changed)
  • Dockerfile can have any name but MUST be in the current directory

Open Questions:

  • If dockerizePip is false but any docker related options are provided, no warning are raised. Should I fix it?
  • Are these limitations an issue? Do you have ideas how to improve on them?

Related issues:

Copy link
Contributor

@dschep dschep left a comment

Choose a reason for hiding this comment

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

Magnifique! I added 2 more related issues to your description 😄

Limitations:

  • build every time is fine since docker's caching should make it fast if nothing has changed.
  • huh. Is that a docker build -f limitation?

Open questions:

  • A warning in that scenario would be great 👍
  • I won't reject this because of those limitations, but I'm not sure how to mitigate them either.

lib/pip.js Outdated
this.serverless.cli.log(`Docker Image: ${this.options.dockerImage}`);
let dockerImage = this.options.dockerImage;
if (this.options.dockerFile) {
this.serverless.cli.log(`Building custom docker image from ${this.options.dockerFile}...`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move this docker build logic to a new lib/docker.js module?

@cgrimal
Copy link
Author

cgrimal commented Jan 18, 2018

Thanks for your feedback @dschep!

  • It is indeed possible to place the Dockerfile in a subfolder. I updated the README. That said, you can't have it elsewhere, or Docker will raise a "The Dockerfile must be within the build context (.)" error. FYI, A working solution in command-line is to do docker build -t sls-py-reqs-custom - < /path/to/Dockerfile but I can't make it work with spawnSync. I tried using the input option but I couldn't make it work.

  • I added a warning (not exiting the process) if one of the docker related option is provided with dockerizePip set to false.

  • I moved some of the docker logic to a dedicated lib/docker.js module.

Let me know what you think.

Copy link
Contributor

@dschep dschep left a comment

Choose a reason for hiding this comment

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

Looks fantastic! I'll make a release after work when I can test it.

@dschep dschep merged commit 50bf258 into serverless:master Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants