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

Cleanup Dockerfile #50

Merged
merged 1 commit into from
Oct 6, 2024
Merged

Conversation

pedropombeiro
Copy link
Collaborator

@pedropombeiro pedropombeiro commented Sep 25, 2024

This change tries to minimize the size of the resulting image (by reducing layers, and not installing optional tools), and also allow specifying a brscan-skey.config (which will later allow us to use Python scripts directly).

Comparison between images (new image on the bottom):

image

It went from 1.1 GB to 764 MB (30% reduction with no change in functionality), and efficiency from 97 % to 99 %.

I've also linted the Dockerfile at https://www.fromlatest.io/ and implemented some of the suggestions.

@pedropombeiro pedropombeiro marked this pull request as draft September 25, 2024 20:11
@pedropombeiro pedropombeiro force-pushed the pedropombeiro/cleanup-Dockerfile branch from a6c14e1 to 003a991 Compare September 25, 2024 20:35
@pedropombeiro pedropombeiro marked this pull request as ready for review September 25, 2024 20:40
@pedropombeiro pedropombeiro force-pushed the pedropombeiro/cleanup-Dockerfile branch 5 times, most recently from 686ae0c to 3b43f74 Compare September 30, 2024 20:26
@pedropombeiro pedropombeiro force-pushed the pedropombeiro/cleanup-Dockerfile branch from 3b43f74 to ce05ff2 Compare October 1, 2024 21:56
@PhilippMundhenk
Copy link
Owner

I like it! The size of the image has long bothered me. In fact, it is still massive for what it does. I feel that it should be possible to get a scanner driver under 700MB -.-
One question: Why remove the "EXPOSE"? I always found that a neat little helper...

Copy link
Owner

@PhilippMundhenk PhilippMundhenk left a comment

Choose a reason for hiding this comment

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

Tested and works

@pedropombeiro
Copy link
Collaborator Author

One question: Why remove the "EXPOSE"? I always found that a neat little helper...

The linter says it has been deprecated since a long time. We can keep it if you prefer though.

@PhilippMundhenk
Copy link
Owner

:O Is it? I wasn't aware of that. It's still in the reference. Anyway, let's take it out then. Merging...

@PhilippMundhenk PhilippMundhenk merged commit 8f2f03f into master Oct 6, 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
None yet
Development

Successfully merging this pull request may close these issues.

2 participants