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

Replace ADD with COPY instruction in Dockerfile #331

Merged

Conversation

PeterDaveHello
Copy link
Contributor

Reference:

For other items (files, directories) that do not require ADD’s
tar auto-extraction capability, you should always use COPY.

Reference:

- https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#add-or-copy

> For other items (files, directories) that do not require `ADD`’s
> tar auto-extraction capability, you should always use `COPY`.
@codecov
Copy link

codecov bot commented Dec 20, 2020

Codecov Report

Merging #331 (ebb0884) into master (2e5b46b) will increase coverage by 0.32%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
+ Coverage   68.56%   68.88%   +0.32%     
==========================================
  Files           8        8              
  Lines         474      495      +21     
==========================================
+ Hits          325      341      +16     
- Misses        149      154       +5     
Impacted Files Coverage Δ
safety/cli.py 50.00% <0.00%> (-6.85%) ⬇️
safety/errors.py 100.00% <0.00%> (ø)
safety/constants.py 100.00% <0.00%> (ø)
safety/safety.py 92.96% <0.00%> (+6.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e5b46b...ebb0884. Read the comment docs.

@rafaelpivato
Copy link
Contributor

Good point about ADD vs COPY. Nevertheless, I am not sure we need this. It would impact cached layers without actually fixing any bug. The best moment to make this change would be when we happen to change the layers for this image. You can consider adding this as part of #328

@PeterDaveHello
Copy link
Contributor Author

The cache impact will be only one time, as I'm still working on #328 and they are independent, maybe we can get this merged first?

Copy link
Contributor

@rafaelpivato rafaelpivato left a comment

Choose a reason for hiding this comment

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

Lets use that opportunity to do that then. One thing I would like to ask is to avoid scattered pull-requests with just a few lines. Group related fixes in one single pull-request next time, please.

@rafaelpivato rafaelpivato merged commit 4a8d983 into pyupio:master Dec 27, 2020
@PeterDaveHello PeterDaveHello deleted the RefactorDockerfileInstruction branch December 27, 2020 15:06
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.

None yet

2 participants