New scripts for building the Zend Server Docker images to make the usa…

[rainviigipuu]

…ge faster and more convenient

[yosifkit]

High level pass, still trying to dig into all the changes.

+RUN /root/ZSinstall.sh $zs_ver $zs_php $zs_flavor $image $tag $zs_build

It may be tempting, for the sake of brevity, to put complicated initialization details into a standalone script and merely add a RUN command in the Dockerfile. However, this causes the resulting Dockerfile to be overly opaque, and such Dockerfiles are unlikely to pass review.

- https://github.com/docker-library/official-images/tree/0b4b8bb40ae57035527afb62376c7daf60716d73#clarity

---

nginx_signing.key

Even though keyservers can be flakey, we recommend using them instead of embedding a key file in the build context. (Image Build
and https://github.com/docker-library/faq/#openpgp--gnupg-keys-and-verification)

[yosifkit]

tail -f $0 > /dev/null 2>&1 &
wait

Since official-images are meant to be prime examples of Dockerization, we do not want to have hacky solutions like bash scripts to be used as a supervisor. Users will latch onto the tail ... &; wait for their own software and not understand the broader implications regarding signal forwarding or daemon monitoring. Instead, something designed to be init should be used ( s6, supervisord, etc).

[rainviigipuu]

I did the change that the repository keys are loaded from keyserver.ubuntu.com.

Are the other two currently up comments something we need to address before it can be merged to official image or can we address them with upcoming changes?

[yosifkit]

They do need to be addressed.

[rainviigipuu]

We believe that all the comments are addressed with the latest commit. Is there something else we can/should do to make the review process easier?

[yosifkit]

As is usual in software development, smaller, incremental changes are much simpler to review and merge. Adding/changing/removing many files in the docker build context (the stuff we review) at once increases our required review effort immensely and with enough changes, basically becomes equivalent to a new image where every line of every file needs review. This usually means the PR will get the lowest priority, since it will require large blocks of dedicated time (like new images 😢😞).

So, without digging in to everything, here are a few things I notice:

• The new .run files in the build context definitely don't help with review, since they contain embedded binary in a text file and so the diff is populated with tons of nonsense. They are also not something we need to necessarily review since they aren't written as part of the Dockerization and are just an artifact of a different upstream. While it might be the opposite of some build systems, we recommend that anything that is its own project, be downloaded, verified, and installed through the Dockerfile, rather than committed to the build context.

• We recommend using gpg directly (Update Git #8881 (comment))

• apt-get clean is a no-op (Add liquibase as official image #8409 (comment)), maybe you meant to do rm -rf /var/lib/apt/lists/*

• RUN lines are normally run via sh -c, so this bash -c is superfluous

  RUN bash -c 'apt-get update && apt-get -y install curl wget sqlite3 nano net-tools gnupg adduser && apt-get clean'

[rainviigipuu]

Thank you for the recommendations - indeed we had some leftovers from previous generations of the image. Removed ‘bash -c’, removed ‘apt-get clean’ (thanks for the apt/lists tip), replaced apt-key with gpg. We have also re-packaged the .run files in tarballs (which they are anyway) - you’re right, we did not consider that diffs would become unreadable.

[rainviigipuu]

Diff for 5b98408:

cp: cannot stat 'tar/php-zendserver_2019.0.4-apache/extensions/${SWOOLE}.run.tgz': No such file or directory

I am unable to reproduce this while building and running images. Is it possible to get more information how to reproduce this?

[yosifkit]

Our diffing script, diff-pr.sh, filters each build context for only files referenced by the Dockerfile to make review easier and cannot handle COPY lines with variables in the source filename.

[rainviigipuu]

Our diffing script, diff-pr.sh, filters each build context for only files referenced by the Dockerfile to make review easier and cannot handle COPY lines with variables in the source filename.

I tried to run the diff-pr.sh locally:

• cloned the official-images repository
• did push the updated Dockerfiles to zendtech/php-zendserver-docker repository
• updated the library/php-zendserver file with correct commit hash
• run ./diff-pr.sh 0 php-zendserver

and I still get the error.

Doesn't it suppose to pick up the updated library/php-zendserver file from where I execute the diff-pr.sh?

[github-actions]

Diff for 1ae9ccb:

TODO diff too large for GitHub comment!
See: http://github.com/docker-library/official-images/actions/runs/356041641

[rainviigipuu]

I am just asking for an update, how the review process is going - is it going to be finished soon or is there still a lot of work to do?

I am asking that, because if there is still a lot of work ahaed of it, then maybe it would be wise for us to close that pull request and we try to explore the possibilities to create smaller set of changes that would be easier and quicker to review to get the images updated?

But if you are almost on the finishing line then it wouldn't be necessary :)