Optimize build image size #5611
Conversation
|
View a preview at https://prompt.ws/r/Azure/azure-cli/5611 |
troydai
left a comment
troydai
left a comment
There was a problem hiding this comment.
Thank you for the contribution. Please see the comments for suggested changes.
Dockerfile
Outdated
There was a problem hiding this comment.
Since the essence of this change is to combine all the installation commands in one RUN so as to reduce the number of layers, it would be nice to also gather all the comments and put them ahead of the RUN command instead of spreading them between the lines.
There was a problem hiding this comment.
Surely it is way more useful to see comments in close proximity to the code it is commenting on?
I would rather remove the comments then move them into one large uber comment.
There was a problem hiding this comment.
I think you can remove the comments.
Dockerfile
Outdated
There was a problem hiding this comment.
Style: align all the lines here by adding the same indentation.
.dockerignore
Outdated
There was a problem hiding this comment.
requirements.txt shouldn't be needed for the Docker build so I agree with Troy. No need for this particular change.
derekbekoe
left a comment
derekbekoe
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
Added some comments.
.dockerignore
Outdated
There was a problem hiding this comment.
requirements.txt shouldn't be needed for the Docker build so I agree with Troy. No need for this particular change.
Dockerfile
Outdated
There was a problem hiding this comment.
No need to install requirements.txt.
There was a problem hiding this comment.
This could by my python noobness, but I though the intention of the requirements.txt file was similar to packages.config for nuget.
If almost all python projects follow this approach, why do this one differently?
Dockerfile
Outdated
There was a problem hiding this comment.
Change to JP_VERSION. It's not always going to be the latest.
Dockerfile
Outdated
There was a problem hiding this comment.
Why remove the pyc files?
They'll likely be created again on first run and may slow down first run performance.
There was a problem hiding this comment.
You are correct. It will run quicker if we don't delete pyc files.
I guess as we are doing "phoenix builds" and the bytecode is python vm dependent (which is not going to change) we are fine to leave the pyc files in place.
On the other hand if you do delete .pyc and .pyo files, you get smaller docker size and are not executing arbitrary bytecode (someone might slip in a dodgy pyc file)
Dockerfile
Outdated
There was a problem hiding this comment.
I think this installs apk packages that are required for runtime.
http://wiki.alpinelinux.org/wiki/Creating_an_Alpine_package
After the package is successfully compiled and created we should make sure it didn't link to any package that is not present in the $depends variable. We do this by using scanelf.
Upgrade to latest dependencies
|
@derekbekoe @taliesins I made additional changes to resolve the conflict. Please take a look and we should get this merged. |
|
Perhaps a rebase? 350000 changes could be troublesome to review. Would be nice to get this merged in 😀 |
|
@taliesins I did some verification and it looks good. Thanks for the change. |
| | xargs -r apk info --installed \ | ||
| | sort -u \ | ||
| )" \ | ||
| && apk add --virtual .rundeps $runDeps \ |
There was a problem hiding this comment.
This line makes no sense if we don't remove .rundeps later?
While cmd:, so: and pc: packages are automatically created virtuals, you can create your own as well. These allow for quick removal of purpose-specific packages. -- https://docs.alpinelinux.org/user-handbook/0.1a/Working/apk.html#_virtuals
5 participants
Optimize build image size
Upgrade to latest dependencies