Skip to content

fix: exclude non source files in .dockerignore#3387

Merged
onbjerg merged 3 commits intoparadigmxyz:mainfrom
paolofacchinetti:main
Jun 26, 2023
Merged

fix: exclude non source files in .dockerignore#3387
onbjerg merged 3 commits intoparadigmxyz:mainfrom
paolofacchinetti:main

Conversation

@paolofacchinetti
Copy link
Contributor

Existing issue

Right now the COPY . . instruction in the Dockerfile copies pretty almost everything from the repository into the build container (including the .git/ directory, the Dockerfile, the README, etc.) which causes two things:

  • slower builds (need to copy everything into the build container)
  • Docker build cache invalidation even when files that aren't source code are changed. This leads to full rebuilds of the reth binary instead of using the existing cached build layer

Solution

To fix this, I changed the /.dockerignore file to exclude all but the source files required at build-time. I've tested that this works and produces a working Docker container, while solving the issues i wrote above.

This approach to exclude all but the source files means that when new files are added to the repo that are not source files, they will be ignored by the COPY . . instruction. New files or directories that are required during the build stage can be whitelisted in the .dockerignore file.

@mattsse mattsse requested a review from rkrasiuk June 26, 2023 14:15
@mattsse mattsse added the A-cli Related to the reth CLI label Jun 26, 2023
@onbjerg
Copy link
Collaborator

onbjerg commented Jun 26, 2023

@paolofacchinetti Please check that this does not break Dockerfile.cross as that is what we use to build releases

@onbjerg onbjerg added C-enhancement New feature or request A-meta Changes in the contributor workflow and planning and removed A-cli Related to the reth CLI labels Jun 26, 2023
Copy link
Collaborator

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

(see above)

.dockerignore Outdated
!Cargo.toml
!Cross.toml
!deny.toml
!Makefile No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please include the licenses too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI right now (before my PR) the licenses are copied during the reth binary build stage of the Dockerfile, but are not copied into the final runner container. Which means that the end user that pulls the reth image won't find the licenses in it. It seems pointless to me to copy the licenses during build time only, would you like that I add them to the final runner container by copying them across the various build stages?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be ideal

Copy link
Collaborator

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #3387 (1ddf70d) into main (30a1ad2) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3387      +/-   ##
==========================================
- Coverage   69.40%   69.39%   -0.01%     
==========================================
  Files         537      537              
  Lines       72018    71991      -27     
==========================================
- Hits        49981    49957      -24     
+ Misses      22037    22034       -3     
Flag Coverage Δ
integration-tests 16.33% <ø> (+0.01%) ⬆️
unit-tests 64.43% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 17 files with indirect coverage changes

@onbjerg onbjerg enabled auto-merge June 26, 2023 16:13
@onbjerg onbjerg added this pull request to the merge queue Jun 26, 2023
Merged via the queue into paradigmxyz:main with commit 6d4fe27 Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-meta Changes in the contributor workflow and planning C-enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants