Skip to content

Conversation

@zwangsheng
Copy link
Contributor

[REFACTOR] Switch docker base image to Eclipse Temurin

What changes were proposed in this pull request?

To close #508 & #381

Why are the changes needed?

According to docker-library/openjdk#505

What are the items that need reviewer attention?

Related issues.

Related pull requests.

How was this patch tested?

/cc @pan3793

/assign @main-reviewer

apt install -y bash tini sshpass openssh-client vim bind9-utils telnet net-tools procps && \
rm /bin/sh && \
ln -sv /bin/bash /bin/sh && \
echo "auth required pam_wheel.so use_uid" >> /etc/pam.d/su && \
Copy link
Member

Choose a reason for hiding this comment

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

grant the current user for su?

RUN chmod +x /usr/bin/tini
RUN set -ex && \
apt-get update && \
ln -s /lib /lib64 && \
Copy link
Member

Choose a reason for hiding this comment

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

why do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to apache/spark#25255 this PR, maybe we don't need this.

ADD https://github.com/krallin/tini/releases/download/${TINI_VERSION}/tini /usr/bin/tini
RUN chmod +x /usr/bin/tini
RUN set -ex && \
apt-get update && \
Copy link
Member

Choose a reason for hiding this comment

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

do we want to use aliyun mirror?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 for build speed

@pan3793
Copy link
Member

pan3793 commented Sep 2, 2022

maybe arthas could be included in the docker image as well, WDYT? @waitinfuture @AngersZhuuuu

@waitinfuture
Copy link
Contributor

ping @FMX @fanyilun

@waitinfuture
Copy link
Contributor

Has this been tested?

@zwangsheng
Copy link
Contributor Author

Has this been tested?

I had build docker images by this Dockerfile.

I will test in kubernetes env. And pause some screenshoot later.

@fanyilun
Copy link
Contributor

fanyilun commented Sep 2, 2022

I meet a problem in this Dockerfile:
/bin/sh: nslookup: command not found
should we add apt install dnsutils ?

@zwangsheng zwangsheng marked this pull request as draft September 7, 2022 02:13
@zwangsheng zwangsheng marked this pull request as ready for review September 7, 2022 03:32
@zwangsheng
Copy link
Contributor Author

popo_2022-09-07  09-52-52
popo_2022-09-07  11-32-31
Tested In Kubernetes Cluster

  • Dockerfile build success
  • Helm Install success

Copy link
Contributor

@waitinfuture waitinfuture left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@waitinfuture waitinfuture merged commit 9152906 into apache:main Sep 7, 2022
@waitinfuture
Copy link
Contributor

Please have a test ping @fanyilun

@fanyilun
Copy link
Contributor

fanyilun commented Sep 8, 2022

It looks good to me now, thanks.

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.

[REFACTOR] Switch docker base image to Eclipse Temurin

4 participants