Skip to content

Use smaller images (Node.js, Python, Ruby) #1749

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 3, 2019
Merged

Use smaller images (Node.js, Python, Ruby) #1749

merged 1 commit into from
Sep 3, 2019

Conversation

@PicardParis
Copy link
Contributor

@PicardParis PicardParis commented Aug 29, 2019

No description provided.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 29, 2019
@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented Aug 29, 2019

Hi @PicardParis. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow-robot knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 29, 2019
grayside
grayside reviewed Aug 29, 2019
View reviewed changes
docs/serving/samples/hello-world/helloworld-nodejs/Dockerfile

# Copy local code to the container image.
COPY . .
COPY . ./
Copy link
Contributor

@grayside grayside Aug 29, 2019

Choose a reason for hiding this comment

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

Why add the slash after the second period? Is this a syntax preference or do you have a functional goal?

Copy link
Contributor Author

@PicardParis PicardParis Aug 30, 2019

Choose a reason for hiding this comment

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

This is a syntax preference.

2 pros

  • All destination folders end with a slash (doc consistency).
  • This will prevent some developers from falling into this dockerfile syntax trap where COPY file1 . works but COPY file1 file2 . fails. Additionally, the sample pattern COPY *.ext . (without /) will eventually bite (only works if there's exactly 1 .ext file). This is used with *.csproj in the C# sample. Using COPY *.ext ./ also prevents this kind of issues from appearing indirectly.

1 con

  • COPY . . looks nicer than COPY . ./.

Copy link
Contributor Author

@PicardParis PicardParis Aug 30, 2019

Choose a reason for hiding this comment

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

Added a comment in #1732 for the potential issue with COPY *.csproj ..

docs/serving/samples/hello-world/helloworld-nodejs/Dockerfile
# Use the official lightweight Node.js 12 image.
# https://hub.docker.com/_/node
FROM node:12
FROM node:12-slim
Copy link
Contributor

@grayside grayside Aug 29, 2019

Choose a reason for hiding this comment

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

I like the choice of slim here, as it keeps the container closer to what developers expect.

Copy link
Contributor Author

@PicardParis PicardParis Aug 30, 2019

Choose a reason for hiding this comment

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

Many developers are going to copy/paste these samples and adapt/use them in production. The motivation for this PR is to use images that:

  • are smaller (x5-10)
  • have less known vulnerabilities

For example, with this Node.js change:

  • Image is x6 smaller (334 → 54.7 MB)
  • There are x7 less detected CVEs (674 → 91)

@grayside
Copy link
Contributor

grayside commented Aug 29, 2019

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 29, 2019
mattwelke
mattwelke suggested changes Aug 30, 2019
View reviewed changes
@PicardParis PicardParis changed the title Use smaller images (Node.js, Python, Ruby, .NET) Use smaller images (Node.js, Python, Ruby) Aug 30, 2019
@PicardParis
Copy link
Contributor Author

PicardParis commented Sep 3, 2019

/assign @mattmoor

mattmoor
mattmoor reviewed Sep 3, 2019
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2019
@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented Sep 3, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, PicardParis

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit 34220a9 into knative:master Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carieshmarie carieshmarie Awaiting requested review from carieshmarie

@gyliu513 gyliu513 Awaiting requested review from gyliu513

3 more reviewers

@grayside grayside grayside left review comments

@mattmoor mattmoor mattmoor left review comments

@mattwelke mattwelke mattwelke requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@grayside grayside

@mattmoor mattmoor

Labels

cla: yes Indicates the PR's author has signed the CLA. kind/serving lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@PicardParis @knative-prow-robot @grayside @mattmoor @mattwelke @googlebot @RichieEscarez