ltb-project / self-service-password

Web interface to change and reset password in an LDAP directory
https://self-service-password.readthedocs.io/en/latest/
GNU General Public License v3.0
1.18k stars 327 forks source link

修改了 Dockerfile 文件 | Modified Dockerfile #932

Closed findlayfeng closed 4 months ago

findlayfeng commented 5 months ago
davidcoutadeur commented 5 months ago

That's quite a big change. I have planned to make a review of this next week.

davidcoutadeur commented 4 months ago

Hello,

First, thank you for your contribution. There is a lot of things inside, and I didn't have time to review all of them from now.

Here are some general remarks:

  1. could you rebase your work on top of master please?
  2. it seems that docker doesn't like the parameter expansion containing dots (.):
Step 3/45 : ARG SMARTY_DIR=/usr/share/php/smarty${SMARTY_VERSION%%.*}
failed to process "/usr/share/php/smarty${SMARTY_VERSION%%.*}": missing ':' in substitution
  1. the modifications you propose seems to require Buildkit. This is unavailable by default on my debian 12:
the --mount option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled

It turns out it's not so complicated to enable, but I don't know if it's a good thing to add a new complexity level:

DOCKER_BUILDKIT=1 docker build -t self-service-password -f ./docker/Dockerfile ../

Do you have any reason for adding buildkit?

  1. I have built a new image, but it is 779MB large. Previous image is 380.54 MB large. Can you try reducing the image size?
findlayfeng commented 4 months ago

Hello,

First, thank you for your contribution. There is a lot of things inside, and I didn't have time to review all of them from now.

Here are some general remarks:

  1. could you rebase your work on top of master please?
  2. it seems that docker doesn't like the parameter expansion containing dots (.):
Step 3/45 : ARG SMARTY_DIR=/usr/share/php/smarty${SMARTY_VERSION%%.*}
failed to process "/usr/share/php/smarty${SMARTY_VERSION%%.*}": missing ':' in substitution
  1. the modifications you propose seems to require Buildkit. This is unavailable by default on my debian 12:
the --mount option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled

It turns out it's not so complicated to enable, but I don't know if it's a good thing to add a new complexity level:

DOCKER_BUILDKIT=1 docker build -t self-service-password -f ./docker/Dockerfile ../

Do you have any reason for adding buildkit?

  1. I have built a new image, but it is 779MB large. Previous image is 380.54 MB large. Can you try reducing the image size?

1 -> 可以的 2&3 -> 用同一个方法解决,更新到最新的 docker get-docker, 2 的语法在新版本已经被支持,3在新版本中是默认开启的 https://docs.docker.com/build/buildkit/#getting-started buildx 是新版本的默认builder 4 将镜像上传到 hub.docker.com 的时候会对镜像进行压缩, 779MB 是压缩前大小 380.54 MB 是压缩后大小,旧的镜像在压缩前 大约为 1.11GB,或者尝试一下 Dockerfile.alpine

1 -> Yes 2&3 -> Use the same method to solve, update to the latest docker get-docker, the syntax of 2 is already supported in the new version, 3 is enabled by default in the new version https://docs.docker.com/build/buildkit/#getting-started buildx is the default builder of the new version 4 -> When uploading the image to hub.docker.com, the image will be compressed. 779MB is the size before compression and 380.54 MB is the size after compression. The old image is about 1.11GB before compression. Or try Dockerfile.alpine

findlayfeng commented 4 months ago

至于使用 --mount option 参数的好处是加速二次构建的速度,对于开发者来说,这是一个非常好的特性,对于用户无差别

The benefit of using the --mount option parameter is that it speeds up the secondary build. For developers, this is a very good feature, and it does not matter to users.

findlayfeng commented 4 months ago

Hello, First, thank you for your contribution. There is a lot of things inside, and I didn't have time to review all of them from now. Here are some general remarks:

  1. could you rebase your work on top of master please?
  2. it seems that docker doesn't like the parameter expansion containing dots (.):
Step 3/45 : ARG SMARTY_DIR=/usr/share/php/smarty${SMARTY_VERSION%%.*}
failed to process "/usr/share/php/smarty${SMARTY_VERSION%%.*}": missing ':' in substitution
  1. the modifications you propose seems to require Buildkit. This is unavailable by default on my debian 12:
the --mount option requires BuildKit. Refer to https://docs.docker.com/go/buildkit/ to learn how to build images with BuildKit enabled

It turns out it's not so complicated to enable, but I don't know if it's a good thing to add a new complexity level:

DOCKER_BUILDKIT=1 docker build -t self-service-password -f ./docker/Dockerfile ../

Do you have any reason for adding buildkit?

  1. I have built a new image, but it is 779MB large. Previous image is 380.54 MB large. Can you try reducing the image size?

1 -> 可以的

2&3 -> 用同一个方法解决,更新到最新的 docker get-docker, 2 的语法在新版本已经被支持,3在新版本中是默认开启的 https://docs.docker.com/build/buildkit/#getting-started buildx 是新版本的默认builder 4 将镜像上传到 hub.docker.com 的时候会对镜像进行压缩, 779MB 是压缩前大小 380.54 MB 是压缩后大小,旧的镜像在压缩前 大约为 1.11GB,或者尝试一下 Dockerfile.alpine 1 -> Yes 2&3 -> Use the same method to solve, update to the latest docker get-docker, the syntax of 2 is already supported in the new version, 3 is enabled by default in the new version https://docs.docker.com/build/buildkit/#getting-started buildx is the default builder of the new version 4 -> When uploading the image to hub.docker.com, the image will be compressed. 779MB is the size before compression and 380.54 MB is the size after compression. The old image is about 1.11GB before compression. Or try Dockerfile.alpine

官方的文档并没有说明具体是哪个版本开始被默认使用 https://docs.docker.com/build/architecture/#buildx

The official documentation does not specify which version is used by default https://docs.docker.com/build/architecture/#buildx

davidcoutadeur commented 4 months ago

2&3 -> Use the same method to solve, update to the latest docker get-docker, the syntax of 2 is already supported in the new version, 3 is enabled by default in the new version https://docs.docker.com/build/buildkit/#getting-started buildx is the default builder of the new version

Thank you for the pointer (and for your answer).

However, in my opinion, I am not sure we should require admins to have specific versions of docker for building new images. If so, each need for having the upstream version of docker should be evaluated conscientiously and approved.

update:

For the record, the new image also builds correctly with podman:

podman build -t self-service-password -f ./docker/Dockerfile ../

if we fix the substitution problem first:

Error: reading multiple stages: Missing ':' in substitution: SMARTY_DIR=/usr/share/php/smarty${SMARTY_VERSION%%.*}
findlayfeng commented 4 months ago

2&3 -> Use the same method to solve, update to the latest docker get-docker, the syntax of 2 is already supported in the new version, 3 is enabled by default in the new version https://docs.docker.com/build/buildkit/#getting-started buildx is the default builder of the new version

Thank you for the pointer (and for your answer).

However, in my opinion, I am not sure we should require admins to have specific versions of docker for building new images. If so, each need for having the upstream version of docker should be evaluated conscientiously and approved.

update:

For the record, the new image also builds correctly with podman:

podman build -t self-service-password -f ./docker/Dockerfile ../

if we fix the substitution problem first:

Error: reading multiple stages: Missing ':' in substitution: SMARTY_DIR=/usr/share/php/smarty${SMARTY_VERSION%%.*}

你的想法对的,我认为如果可以在dockerfile中指定支持的语法版本这将都不是问题,可惜现在不能。 我会提供一个兼容的版本。

You are right, I think this wouldn't be a problem if I could specify the supported syntax version in the Dockerfile, but unfortunately I can't do that now. I will provide a compatible version.

davidcoutadeur commented 4 months ago

Thanks, with last commit it should be ok.

I have made a full review of Dockerfile and install and entrypoint.sh files.

Globally it seems ok. There are many small improvements. I find the install recipe a bit heavy, but if some day we have a more complex project, we could do customization there.

@findlayfeng some more remarks:

findlayfeng commented 4 months ago

@davidcoutadeur 抱歉 在 添加 Dockerfile.alpine 后添加了一些处理脚本,这些脚本没有被完整的测试,现在是按照预期的工作了

Sorry, added some processing scripts after adding Dockerfile.alpine, these scripts were not fully tested, now it works as expected

davidcoutadeur commented 4 months ago

Still some minor issues for docker alpine image.

I have fixed these by myself, and added some documentation. I have also rebased and squashed your commits.

The merge request will be done in #947