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

[#1891] improvement(dev): dockerfile remove cache after installing packages #2090

Conversation

zhuzilong2013
Copy link
Contributor

What changes were proposed in this pull request?

Remove cache after installing packages.

Why are the changes needed?

Fix: #1891

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

By hand

justinmclean
justinmclean previously approved these changes Feb 6, 2024
Copy link
Member

@justinmclean justinmclean left a comment

Choose a reason for hiding this comment

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

LGTM

@yuqi1129
Copy link
Contributor

yuqi1129 commented Feb 6, 2024

@xunliu
Can you assist in verifying this PR?

@@ -151,6 +151,10 @@ RUN chown -R datastrato:hadoop /home/datastrato
# removed install packages
RUN rm -rf /tmp/packages

################################################################################
# removed install cache
RUN rm -rf /var/cache/apt/*
Copy link
Member

Choose a reason for hiding this comment

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

hi @zhuzilong2013 Thank you for your contribution.

I think it's best to use the apt clean command to remove the cached files, but this needs to be tested.
https://askubuntu.com/questions/1350375/can-i-delete-var-cache-apt-archives

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment. It's my mistake. I should use rm -rf /var/lib/apt/list/* actually.
In https://docs.docker.com/develop/develop-images/instructions/#run:

Official Debian and Ubuntu images automatically run apt-get clean, so explicit invocation is not required.

I will fix and test it. I also want to move 'mysql-sever' to basic tools, what do you think?

@yuqi1129
Copy link
Contributor

yuqi1129 commented Feb 7, 2024

@zhuzilong2013
Is it ready to be reviewed again?

@zhuzilong2013
Copy link
Contributor Author

@yuqi1129 Please review it:)

@yuqi1129
Copy link
Contributor

yuqi1129 commented Feb 7, 2024

@yuqi1129 Please review it:)

I'm okay with the changes, @xunliu do you have any further comments?

@qqqttt123
Copy link
Contributor

@xunliu @yuqi1129 Gently ping.

@qqqttt123 qqqttt123 requested a review from xunliu February 18, 2024 02:29
Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

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

hi @zhuzilong2013 Thank you for your updated.
Have you tested to correctly create the Docker image using your development branch?

@zhuzilong2013
Copy link
Contributor Author

hello @xunliu , I found the mysql silent installation failed because I moved 'mysql-sever' to basic tools. I added a new ARG to resolve it.
I build image on my M1 MacBook:
截屏2024-02-20 18 02 52
Please review it.

@jerryshao jerryshao requested a review from xunliu February 21, 2024 06:02
@@ -9,6 +9,7 @@ LABEL maintainer="[email protected]"
ARG HADOOP_PACKAGE_NAME
ARG HIVE_PACKAGE_NAME
ARG JDBC_DIVER_PACKAGE_NAME
ARG DEBIAN_FRONTEND=noninteractive
Copy link
Member

Choose a reason for hiding this comment

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

Where use this DEBIAN_FRONTEND arg? I didn't find it anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it because I moved 'mysql-sever' to basic tools.

Copy link
Member

Choose a reason for hiding this comment

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

@jerryshao jerryshao requested a review from xunliu February 27, 2024 04:06
Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

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

LGTM

@xunliu xunliu merged commit d6d6f34 into apache:main Feb 27, 2024
12 checks passed
@mchades
Copy link
Contributor

mchades commented Mar 6, 2024

Has this PR been tested by IT? @xunliu @zhuzilong2013

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.

[Improvement] Dockerfile remove cache after installing packages
6 participants