Refactor images for old Mbed CLI 1 and Mbed CLI 2 #6
Conversation
arekzaluski
force-pushed
the
image-refactor
branch
from
8902229
to
f9610dc
Compare
| WORKDIR /root | ||
| # Bootstrap | ||
| ADD bootstrap.sh /root | ||
| RUN sh /root/bootstrap.sh |
There was a problem hiding this comment.
I really dislike the change to a single command, a change to bootstrap.sh triggers a full rebuild and it is slow with multiarch. Whereas, with various RUN, actions made before the modification are cached.
There was a problem hiding this comment.
I am agree with @pan- here, change it to a single command makes the huge docker image (1.4GB I think) into a single layer, every change will triggers a full rebuilding of whole layer. And this impair the benefit of the layer caching, and make the upload/download a slower process
There was a problem hiding this comment.
Another point is if this bootstrap scripts calling other scripts make changes to the docker image contents, the it would be unclear what it is in the docker image.
One of major benefit of Dockerfile is : It is self-descriptive, by reading the Dockerfile, you know what it is inside image
There was a problem hiding this comment.
The initial idea was to share one script for Docker and others (e.g. Vagrant), but I agree that performance (caching) should be prioritised.
It would be good to have this part reverted, while having other improvements (e.g. installing CMake via PPA) merged soon, so our CI (mainly Travis) can benefit from that.
There was a problem hiding this comment.
Some Travis scripts currently still fetch the raw Ubuntu image and install stuff manually at the moment
There was a problem hiding this comment.
(By "reverted" I mean a separate PR - no need to abandon any work we've done here)
|
|
||
| PARAMS="$PARAMS -it --rm" | ||
| PARAMS="$PARAMS -v $WORKSPACE:$PROJECT_DIR" | ||
| PARAMS="$PARAMS -p $PORT:8080" |
There was a problem hiding this comment.
What is exposed in port 8080?
| WORKSPACE=${@:-$HOME} | ||
| PROJECT_DIR="/work/$(basename $WORKSPACE)" | ||
|
|
||
| PARAMS="$PARAMS -it --rm" |
There was a problem hiding this comment.
I actually think we do not need a a docker_run.sh. We can provide the whole "docker run" command encouraging user to understand what they are doing.
| @@ -0,0 +1,5 @@ | |||
| #!/usr/bin/env sh | |||
There was a problem hiding this comment.
Do we really new a wrapper script? CI will be using "docker buildx" for building multi arch.
|
I am trying to merge the changes to #7
@arekzaluski @LDong-Arm anything else? |
Changes