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

feature: minimize container image #28

Merged

Conversation

simar0at
Copy link
Contributor

Rearrange Dockerfile so it creates as little layers as possible and removes everything temporary in the same RUN step as it is downloaded. Like this these downloads are never in any layer and just hidden by deleting them later.

Rearrange Dockerfile so it creates as little layers as possible and
removes everything temporary in the same RUN step as it is downloaded.
Like this these downloads are never in any layer and just hidden by deleting them later.
* does not play nice with multi platform
* should not contain anything after the build (apt-get clean)
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated
# install nodejs
curl -fsSL https://deb.nodesource.com/setup_18.x -o nodesource_setup.sh && \
bash nodesource_setup.sh && \
apt install nodejs && \
# install prince
Copy link
Member

Choose a reason for hiding this comment

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

@simar0at Can you please check indentation here and in the following lines?

Copy link
Member

Choose a reason for hiding this comment

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

just realised that the apt install nodejs && \ was dropped, can’t recall if or why this was necessary… ideas?

@musicEnfanthen musicEnfanthen changed the title Feature: Minimize container image feature: minimize container image Jan 20, 2024
@musicEnfanthen musicEnfanthen requested a review from bwbohl January 21, 2024 23:58
@musicEnfanthen
Copy link
Member

Beside the indentations, looks good to me (we can fix it later if needed. GH UI seems to be contradicting any fixing attempts). Tested locally as well, runs fine.

@musicEnfanthen
Copy link
Member

Will let @bwbohl have another look.

@bwbohl
Copy link
Member

bwbohl commented Jan 26, 2024

I created a PR with the fixed against @simar0at ’s
Many thanks; that really compresses the image by about 300MB!

I was wondering whether we should also move the download of ANT, Saxon, and Xerces (line 27) into the RUN (line 35). Doing so, we would drop another layer, and this should further reduce image size, correct?

@bwbohl bwbohl added this to the MEI 5.x milestone Jan 26, 2024
Copy link
Member

@bwbohl bwbohl left a comment

Choose a reason for hiding this comment

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

created a PR with the corresponding indentation

@simar0at
Copy link
Contributor Author

Downloading in a big RUN might reduce the size a little but you will get the biggest reduction if you then also delete install packages you downloaded in that RUN statement.

@bwbohl
Copy link
Member

bwbohl commented Jan 26, 2024

Deleting the zips after deploying them would be absolutely fine

@simar0at
Copy link
Contributor Author

145.2 E: Failed to fetch https://packages.adoptium.net/artifactory/deb/pool/main/t/temurin-17/temurin-17-jdk_17.0.10.0.0%2b7_arm64.deb  Error reading from server. Remote end closed connection [IP: 146.75.39.42 443]

Either the logic for getting temurin is broken or this is a one off and just rerunning will du.

@musicEnfanthen
Copy link
Member

Rerun was fine

@musicEnfanthen
Copy link
Member

@bwbohl Ready to merge?

@musicEnfanthen
Copy link
Member

Thank you @simar0at for this helpful contribution!

@musicEnfanthen musicEnfanthen merged commit 2ba6c6c into music-encoding:main Apr 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants