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

fix(image): add logic to detect empty layers #2790

Merged
merged 2 commits into from Aug 30, 2022
Merged

Conversation

DmitriyLewen
Copy link
Contributor

Description

Added logic to detect empty layers.
Logic is taken from buildkit.

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@DmitriyLewen DmitriyLewen changed the title add logic to detect empty layers fix(image): add logic to detect empty layers Aug 30, 2022
@DmitriyLewen DmitriyLewen marked this pull request as ready for review August 30, 2022 08:17
return true
}
// commands here: 'ADD', COPY, RUN and WORKDIR != "/"
// Also RUN command may don't include 'RUN' prefix
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it happen? I thought RUN was required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Also RUN command may don't include 'RUN' prefix
// Also RUN command may not include 'RUN' prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Image created with docker build command may not include command prefix.
Docker docs have this.
e.g.
Dockerfile:

FROM alpine

RUN mkdir test

RUN apk update

History:

➜ docker history 2780:test 
IMAGE          CREATED         CREATED BY                                      SIZE      COMMENT
b06f8dfdd74d   6 seconds ago   /bin/sh -c apk update                           2.29MB    
4d55786712ed   8 hours ago     /bin/sh -c mkdir test                           0B        
0ac33e5f5afa   4 months ago    /bin/sh -c #(nop)  CMD ["/bin/sh"]              0B        
<missing>      4 months ago    /bin/sh -c #(nop) ADD file:5d673d25da3a14ce1…   5.57MB 

Also layers created with buildkit have command prefix:

➜ docker history 2780:test2
IMAGE          CREATED         CREATED BY                                      SIZE      COMMENT
8485e63ed150   5 seconds ago   RUN /bin/sh -c apk update # buildkit            2.29MB    buildkit.dockerfile.v0
<missing>      8 minutes ago   RUN /bin/sh -c mkdir test # buildkit            0B        buildkit.dockerfile.v0
<missing>      4 months ago    /bin/sh -c #(nop)  CMD ["/bin/sh"]              0B        
<missing>      4 months ago    /bin/sh -c #(nop) ADD file:5d673d25da3a14ce1…   5.57MB   

{
name: "ENV",
history: dimage.HistoryResponseItem{
CreatedBy: "/bin/sh -c #(nop) ENV TESTENV=TEST",
Copy link
Collaborator

Choose a reason for hiding this comment

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

BuiltKit doesn't have /bin/sh -c #(nop) prefix. I also want to add such a case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@knqyf263 knqyf263 merged commit 2473b2c into main Aug 30, 2022
@knqyf263 knqyf263 deleted the fix/detect-empty-layers branch August 30, 2022 12:56
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

2 participants