Updates for release 23.0.0.6

[wraschke]

These are updates needed to publish WebSphere and Open Liberty 23.0.0.6 containers. Thank you.

[wraschke]

Okay, cool, now the only check I see failing is one that I've seen in past PRs:

Error: Unhandled error: HttpError: Validation Failed: {"resource":"IssueComment","code":"unprocessable","field":"data","message":"Body is too long (maximum is 65536 characters)"}

Can this PR be merged soon?

[docker-library-bot]

Diff (01a9c94): https://gist.github.com/docker-library-bot/ad65ae404764410e6f8185323938fb9f

Diff (4989655): https://gist.github.com/docker-library-bot/a697fa77cfedd4ef69c1643b4b56c1b8

[wraschke]

@yosifkit can you please give an outlook on when this PR can be merged?

[yosifkit]

Apologies for the delay, since the Docker official images are curated to ensure their quality, reviews on large changes often get pushed back in favor of the shorter/quicker changes. Large changes require larger contiguous blocks of time for us to understand and provide relevant feedback and that can tax our small team.

Generally, changes to files in the build context should be kept minimal to help keep our build context diffs easy to read so that we can continue to review quickly. If it becomes a large file that changes every release, it may make sense to include it in the upstream release (or released as a fully maintained project/example that can be downloaded by the image).

Here is some feedback that I have so far:

We try hard to avoid multi-stage builds in almost every instance. Since this image does a multi-stage build to basically prevent some rm's and an apt-get purge --auto-remove ..., this is unfortunately not an acceptable use of a multi-stage build for official-images. (see also https://github.com/docker-library/faq/#multi-stage-builds).

RUN apt-get install -y --no-install-recommends openssl This has no apt-get update and so would fail, but the package is already installed in the base.

USER root: this only applies to the -ubi images provided by ibm-semuru-runtimes in the IBM registry (ibmruntimes/semeru-containers#57), so I am unsure why it was also applied to the Ubuntu based images. If they wanted to add the same USER to the Ubuntu-based images in Docker official images, we'd likely push back a little because of this same breakage for users.

[mbroz2]

@yosifkit thanks for the feedback and the explanation regarding the process. We'll do our best to keep the changes to a minimum moving forward to help keep the diffs small. These past releases have been a bit of an abnormality due to new functionality being included, as well as some changes with regards to how we build the images.

Regarding the multi-stage builds, the reason we switched to them is to allow a local copy of the runtime (or repository) to be used at build time (the COPY resources/ /tmp/ step), rather then needing to download one. This change was done for several reasons, but the main benefits of this approach apply to the UBI based images. Our primary reason for including this change in the Ubuntu based images was to avoid diverging the UBI and Ubuntu based Dockerfiles, though users building their own Liberty Ubuntu based images using these Dockerfiles could leverage some of the benefits as well. We were unaware that this could cause problems for building the official images, and if necessary, can revert back to a single stage build.

[leochr]

@yosifkit thank you for the review and feedback.

Regarding USER root, it's good to know you will likely push back on Semeru image changing the user, but if it's alright we would like to explicitly switch to root since some of the instructions in our Dockerfiles should be run as root (we later switch to non-root user). It'll make that explicit/clear to anyone looking at our Dockerfiles and also keep the Dockerfiles aligned with UBI files.

[mbroz2]

@yosifkit based on the two comments above, how would you suggest we proceed regarding the multi-stage build and USER root?

[yosifkit]

We'd rather have them as single stage builds, but the USER root is harmless.

[mbroz2]

understood. we'll make the update to revert back to a single stage build.

[wraschke]

@yosifkit can you take another look? Thank you.

[yosifkit]

🤔 Uncertain if this should be closed in favor of #15098 since it seems to contain the updates here. It is failing to build the new version, so I figured I'd finish reviewing here first.

LABEL, ENV should be defined at late as possible so that their changing wouldn't break unnecessary build cache. Related is the location of the RUN that installs dumb-init, it should be before the LABEL and ENV declarations to maximize the cache efficiency of the Dockerfile (i.e. installed packages/binaries that don't change because of a Open/WebSphere-liberty version bump should be defined in layers before any version-related ENV or LABEL). I believe that there are already versions that have the dumb-init install line later than the liberty version variables, so this is instead something to be considered instead of blocking this PR, but may be included if you like.

LGTM, let me know if we should merge this or close this for #15098 instead to get both updates.

[leochr]

@yosifkit Thank you for the review. Please merge this PR to publish 23.0.0.6 images. We'll investigate the failures in #15098 and consider the above comment about LABEL, ENV