-
Notifications
You must be signed in to change notification settings - Fork 6k
Update docker image to match the LUCI and Framework images #25009
Conversation
Do not review.
cbracken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I think there's an apt-get install google-chome-stable missing.
|
|
||
| # Add repo for chrome stable | ||
| RUN wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | apt-key add - | ||
| RUN echo 'deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main' | tee /etc/apt/sources.list.d/google-chrome.list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not need to install google-chrome-stable here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
ci/docker/build/Dockerfile
Outdated
| dosfstools ifupdown libedit-dev libglib2.0-dev liblz4-tool libncurses5-dev libssl-dev lsof mtools nasm net-tools python2.7-dev \ | ||
| python-m2crypto tcpdump texinfo uuid-dev vim xz-utils zlib1g-dev firefox-esr g++-multilib libblkid-dev libc6-dev-i386 \ | ||
| libfreetype6-dev libegl1-mesa libgles2-mesa-dev libglu1-mesa-dev libgtk-3-dev libx11-dev libxcursor-dev libxinerama-dev libxrandr-dev \ | ||
| libxxf86vm-dev mesa-common-dev openjdk-8-jdk zip xvfb ca-certificates gnupg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this patch, but out of curiosity does this all need to be a single RUN command?
I wonder if it would be easier to follow if we broke into sections (in a followup patch) like:
- apt-get update
- Install gpg to support adding repos
- Add additional repos like chrome, gcloud
- apt-get update
- install basic tools like wget/curl/.../autoconf/.../groff/.../build-essential/tcp-dump
- Install X/GL build deps like libglib2.0-dev etc.
- Install browsers: chrome and firefox
- do gcloud config
- clone depot_tools
- clone buildroot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
| gke_container: | ||
| dockerfile: "ci/docker/build/Dockerfile" | ||
| builder_image_name: docker-builder # gce vm image | ||
| builder_image_project: flutter-cirrus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this? is this the gcp project that owns the storage bucket we upload images to? or the project that owns the docker-builder vm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the project the docker image is uploaded/downloaded from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, i found the project
| apt-get update && apt-get install -y google-cloud-sdk google-chrome-stable libx11-dev && \ | ||
|
|
||
| # Updates the distribution. | ||
| RUN apt-get update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not a big deal, but the reason we did so many steps in a single RUN statement was to reduce the overall docker image size. each dockerfile statement is saved as its own image layer. if you download a file in one layer and then delete it in a subsequent layer, your final image will have the file size times two, whereas if you download and delete it in a single layer, the net file storage will be zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, do you know how can I check the sizes to compare? if the increase is significant I'll get it back to multiple statements per run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can pull the image you just created via: docker pull gcr.io/flutter-cirrus/35f4dd0e84cf343e53d000bce8899df0:latest. when I tried it locally I didn't have permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sizes are about the same:
REPOSITORY TAG IMAGE ID CREATED SIZE
gcr.io/flutter-cirrus/35f4dd0e84cf343e53d000bce8899df0 latest 76f5b861a37a 18 hours ago 2.57GB
gcr.io/flutter-cirrus/df42c0c510954e88f1278196db0afdce latest b9ce201299b9 4 months ago 2.38GB
0.19GB increase do you think is a good tradeoff for the readability of the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
christopherfujino
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…5009) * Test docker image update. Do not review. * Test build in a VM. * Add project name. * Update image type. * Add apt-get dependencies. * Update dependencies. * Update gcloud packages installation. * Update dockerfile. * Fix errors in dockerfile. * Fix dockerfile. * Organize docker file to make it more readable.
…5009) * Test docker image update. Do not review. * Test build in a VM. * Add project name. * Update image type. * Add apt-get dependencies. * Update dependencies. * Update gcloud packages installation. * Update dockerfile. * Fix errors in dockerfile. * Fix dockerfile. * Organize docker file to make it more readable.
…5009) * Test docker image update. Do not review. * Test build in a VM. * Add project name. * Update image type. * Add apt-get dependencies. * Update dependencies. * Update gcloud packages installation. * Update dockerfile. * Fix errors in dockerfile. * Fix dockerfile. * Organize docker file to make it more readable.

Updates the docker image to use debian:stretch to match LUCI and Framework. This is in preparation to improve Cirrus CI usage at Flutter.
Bug: flutter/flutter#77624