docker icon indicating copy to clipboard operation
docker copied to clipboard

Publish to official?

Open loynoir opened this issue 3 years ago • 3 comments

I guess it's something like

https://github.com/docker-library/official-images/tree/master/library

https://github.com/docker-library/official-images/blob/master/library/postfixadmin

loynoir avatar Mar 10 '22 14:03 loynoir

Will have to look into this.

cmouse avatar Dec 19 '22 07:12 cmouse

Hi, this is just a review from a fellow docker official image maintainer in the hope that when this gets to review its going to be easier and quicker to approve:

Per https://github.com/docker-library/official-images/blob/master/NEW-IMAGE-CHECKLIST.md

  • The license of Attribution-NonCommercial-ShareAlike 4.0 International + exemption doesn't appear to be OSI approved.
  • missing a test - https://github.com/docker-library/official-images/tree/master/test/tests

on https://github.com/docker-library/official-images/blob/master/README.md#review-guidelines

  • https://github.com/docker-library/official-images/blob/master/README.md#consistency while bash is there, if you remove tini it isn't. Existing practices use a docker-entrypoint.sh script to achieve this
  • as dovecot -F is essentially doing the same as tini may was well use that. It doesn't process SIGQUIT, but I think the SIGTERM which works fine.
  • "make-ssl-cert generate-default-snakeoil" would be generating the same certs for every container. Is this right? or should it be wrapped in an entrypoint and called to generate files somewhere in a volume.
  • keeping to a directory for 2.3 releases would make it easier to review, when a repo is update to a new release, old container images still exist. A buildarg of the container version and use that to pin the package version installed would be repeatable.
  • a single function per container might be useful, so a container can just run imap, maybe this is the CMD for the container as specified by the user and processed by the entrypoint.
  • DEBIAN_FRONTEND=noninteractive apt-get install ... for minimal scope.
  • container=docker - unsure why
  • LC_ALL=C unsure why. Thought build environment was clean with respect to this.

grooverdan avatar Dec 15 '23 04:12 grooverdan

Hi! Thank you for the informal review, some comments for it:

  • License is applied to these Dockerfiles only, the license for the software is OSI compatible.
  • We can try add some simple tests
  • tini is used to ensure that we don't get zombies in certain situations.
  • make-ssl-cert: you should provide your own certs in any case. these are only to get dovecot running.
  • not sure what you mean with "keeping a directory for 2.3 releases".
  • single function per container: it is unlikely useful to run just imap as it requires stdin input then.

cmouse avatar Dec 15 '23 09:12 cmouse