Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an official image for Plone #1434

Merged
merged 25 commits into from Oct 12, 2016
Merged

Add an official image for Plone #1434

merged 25 commits into from Oct 12, 2016

Conversation

avoinea
Copy link
Contributor

@avoinea avoinea commented Feb 11, 2016

Checklist for Review

NOTE: This checklist is intended for the use of the Official Images maintainers both to track the status of your PR and to help inform you and others of where we're at. As such, please leave the "checking" of items to the repository maintainers. If there is a point below for which you would like to provide additional information or note completion, please do so by commenting on the PR. Thanks! (and thanks for staying patient with us ❤️)

  • associated with or contacted upstream?
  • does it fit into one of the common categories? ("service", "language stack", "base distribution")
  • is it reasonably popular, or does it solve a particular use case well?
  • does a documentation PR exist? (should be reviewed and merged at roughly the same time so that we don't have an empty image page on the Hub for very long)
  • dockerization review for best practices and cache gotchas/improvements (ala the official review guidelines)?
  • 2+ dockerization review?
  • existing official images have been considered as a base? (ie, if foobar needs Node.js, has FROM node:... instead of grabbing node via other means been considered?)
  • if FROM scratch, tarballs only exist in a single commit within the associated history?
  • passes current tests? any simple new tests that might be appropriate to add? (https://github.com/docker-library/official-images/tree/master/test)

@demarant
Copy link

+1 🔌

@avoinea avoinea changed the title Add Plone Add an official image for Plone Mar 23, 2016
@avoinea
Copy link
Contributor Author

avoinea commented Mar 30, 2016

Any update with this PR?

@tianon
Copy link
Member

tianon commented Apr 6, 2016

Sorry for the delay, @avoinea; our backlog has been getting the better of us.

I've been taking a look at the PR this afternoon, and I noticed that it requires some kind of "ZEO server" already running for proper operation -- is that correct? Is the ZEO server something that can be embedded directly into Plone itself, or does it always have to operate standalone? Is it used by projects other than Plone? (when I Google for it, the majority of the hits are Plone-related)

@demarant
Copy link

demarant commented Apr 7, 2016

@tianon
just a quick answer to your question to @avoinea. Plone does not need zeo. If zeo server is not specified zope will run with the built/in dbms. Zeo is needed for clustering of plone, when you want to run multiple plone instances on a single host or mutli-hosts.

if necessary @avoinea can give more details.

otherwise we also have a zeo image ttps://hub.docker.com/r/plone/zeoserver/ maintained by the Plone community. The original Zope foundation behind zeo is kind of not active in zope that much anymore, so we cannot expect an official zeo-image, on the other hand the Plone community one is actively maintaining the zeo docker image.

@avoinea
Copy link
Contributor Author

avoinea commented Apr 7, 2016

As @demarant mentioned ZEO server is just the common and most popular option for clustering Plone deployments. But this can be easily accomplished also with PostgreSQL, MySQL or Oracle using RelStorage.

@psftw
Copy link
Contributor

psftw commented Apr 12, 2016

Nice work. I just looked over the 5.0.4 debian Dockerfile, built it, and briefly toyed with a self-contained (not separate ZEO server) instance. I didn't run in to any issues. I noticed the build takes a while and downloads a lot of build toolchain packages, though I guess there isn't much that can be done to avoid this and it appears to be cleaned up nicely. Since ZEO server is a good default and it's included, I wonder if the image could support both Plone and ZEO depending on the command used?

@avoinea
Copy link
Contributor Author

avoinea commented Apr 13, 2016

@psftw That's an interesting question. I'll see if it's feasible ;)

@avoinea
Copy link
Contributor Author

avoinea commented Apr 13, 2016

@psftw Done.

Now you can:

Start ZEO:

docker run --name=zeo plone zeoserver

Start Plone clients:

docker run -P --link=zeo -e ZEO_ADDRESS=zeo:8100 plone
docker run -P --link=zeo -e ZEO_ADDRESS=zeo:8100 plone

@demarant
Copy link

@psftw @tianon
thanks for your feedback. avoinea has improved it. Could we get this PR accepted soon? the plone community is waiting :)

@psftw
Copy link
Contributor

psftw commented May 17, 2016

@demarant @avoinea sorry for the long delay. I'm happy with where it is at. I appreciate the hard work and for your patience with us. I didn't test the 4.3.7, 4.3.8, 5.0.2 and 5.0.3 tags on the assumption that these are only included in the initial PR to backport those releases. Also, wonder if it's possible to kill off the log output saved to /data/log as it's pretty chatty using default settings and already sent to stdout/err of the container.

@avoinea
Copy link
Contributor Author

avoinea commented May 18, 2016

@psftw I'm afraid that's not possible as the logtail command is reading logs from this file.

@avoinea
Copy link
Contributor Author

avoinea commented May 18, 2016

@psftw or do you mean by using ln -sf technique, like nginx does?

@psftw
Copy link
Contributor

psftw commented May 18, 2016

@avoinea sure, that's one possibility if Plone logs to a consistently named file. Some other images embed a configuration change to achieve similar results. Not sure what the best approach is for Plone specifically.

@avoinea
Copy link
Contributor Author

avoinea commented May 18, 2016

@psftw

Unfortunately the ln -sf doesn't seems to be an option:

daemon process started, pid=16
[Errno 29] Illegal seek

And Plone/Zope doesn't seems to support STDOUT/STDERR only logging. It supports both logging to file and STDOUT/STDERR, but not STDOUT/STDERR only.

The good news is that it is configured to log-rotate, thus it will not unlimited expand the container:

$ cat base.cfg | grep event

# automatic log rotation for event and access logs.
event-log-max-size = 5 MB
event-log-old-files = 5

$ cat base.cfg | grep access

# automatic log rotation for event and access logs.
access-log-max-size = 20 MB
access-log-old-files = 5

@avoinea
Copy link
Contributor Author

avoinea commented Jul 18, 2016

@tianon @psftw

Any status update on this PR?
What is it missing to be merged?

@tianon
Copy link
Member

tianon commented Jul 19, 2016

Sorry for the delay -- our backlog is always longer than we'd like!

I'm a bit concerned by this bit in the entrypoint: (and curious about the other things the entrypoint does)

  $CMD start
  $CMD logtail &

  child=$!
  wait "$child"

Is there not a way to start the server in the foreground? The pattern used here (start the daemon and then wait on the "log tail" process) is usually very fragile. For example, if the server fails to start or terminates unexpectedly, there's no solid indication to the operator of the system that it's done so ( (beyond hoping that the log message provides something meaningful noting service failure and that someone is paying close enough attention to notice and take action; where in the normal Docker flow of tracking the single, foreground process, Docker itself tracks that the process has died and marks the container as exited, potentially also applying restart policies for bringing it back up).

@avoinea
Copy link
Contributor Author

avoinea commented Sep 19, 2016

@tianon Please have a look as this PR is here for ages :)

  • Fixed the docker-entrypoint.sh to watch main process instead of logtail
  • Added tests
  • Updated manifest file to RFC 2822

* Rename BUILDOUT_EGGS -> PLONE_ADDONS, ADDONS
* Rename BUILDOUT_ZCML -> PLONE_ZCML, ZCML
* Rename BUILDOUT_DEVELOP -> PLONE_DEVELOP, DEVELOP
* Add Deprecation Warnings for renamed env variables
@avoinea
Copy link
Contributor Author

avoinea commented Oct 6, 2016

@tianon

Can we speed up a little bit the review of this image as I have a talk about Docker and Plone at Boston Plone Conference 2016 very soon: https://2016.ploneconf.org/talks/docker-and-plone-from-development-to-continuous-deployment

Thank you!

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

It's not ideal to have the shell monitoring the process, but at least now it's monitoring the main process, so it's acceptable (and hopefully there'll be a wishlist item upstream to either allow fg without dev mode or to fix the issues with console? 😇). I've made a few minor comments on this PR that I'd like to see some changes (or clarification), but overall this is looking reasonable 👍

@@ -0,0 +1,43 @@
Maintainers: Alin Voinea alin.voinea@gmail.com (@avoinea),
Sven Strack sven@so36.net (@svx)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add <> around these email addresses please?

ie:

Maintainers: Alin Voinea <alin.voinea@gmail.com> (@avoinea),
             Sven Strack <sven@so36.net> (@svx)


Tags: 5.0.5
GitCommit: 443c34c35e774820621bf5c0966c81c3b45cb284
Directory: 5.0/5.0.5/debian
Copy link
Member

Choose a reason for hiding this comment

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

Are these old versions officially supported upstream, or are you simply looking to backfill the tags and looking to remove them after they're built? (See also https://github.com/docker-library/official-images#tags-and-aliases)

To put it another way, if there's a problem discovered in 5.0.5, will there be a 5.0.5.1 release, or is that what 5.0.6 is? Is there a compelling reason why a user would need to use 5.0.5 and couldn't upgrade to 5.0.6?

When tags are removed here, they do not get removed from the Docker Hub, so are still available for users to pull as desired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if there's a problem discovered in 5.0.5, will there be a 5.0.5.1 release, or is that what 5.0.6 is?

5.0.6 is supposed to fix issues in 5.0.5.

cid="$(docker run -d -e PLONE_DEVELOP=src/eea.facetednavigation -e PLONE_ADDONS=eea.facetednavigation -e PLONE_ZCML=eea.facetednavigation-meta --name "$cname" "$image" cat)"
trap "docker rm -vf $cid > /dev/null" EXIT

docker exec -it "$cname" cat custom.cfg
Copy link
Member

Choose a reason for hiding this comment

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

The -it here will actually cause trouble in some situations where these get run (no TTY available, for example), so this should just be docker exec "$cname" ...

image="$1"

PLONE_TEST_SLEEP=3
PLONE_TEST_TRIES=5
Copy link
Member

Choose a reason for hiding this comment

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

These appear to be unused in this script. 👍

--link "$cname":plone \
--entrypoint python \
"$image" \
-c "import urllib2; con = urllib2.urlopen('$1'); print con.read()"
Copy link
Member

Choose a reason for hiding this comment

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

Indentation here should be tabs, not spaces. 🙏

[[ $(get 'http://plone:8080') == *"Plone is up and running"* ]]

# Create a Plone site
[[ $(get_auth 'http://plone:8080/@@plone-addsite' 'YWRtaW46YWRtaW4=') == *"Create a Plone site"* ]]
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace this magic auth string with "$(echo -n 'admin:admin' | base64)" instead so it's more clear at a glance what this is (and easier to update if the defaults ever change)?

Copy link
Member

Choose a reason for hiding this comment

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

Also, both these tests should be quoted, ie:

# Plone is up and running
[[ "$(get 'http://plone:8080')" == *"Plone is up and running"* ]]

# Create a Plone site
[[ "$(get_auth 'http://plone:8080/@@plone-addsite' "$(echo -n 'admin:admin' | base64)")" == *"Create a Plone site"* ]]

[[ $(get 'http://plone:8080') == *"Plone is up and running"* ]]

# Create a Plone site
[[ $(get_auth 'http://plone:8080/@@plone-addsite' 'YWRtaW46YWRtaW4=') == *"Create a Plone site"* ]]
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as the previous test.

@avoinea
Copy link
Contributor Author

avoinea commented Oct 8, 2016

@tianon

Thank you for the review. I fixed the reported issues.

About the older version, indeed we can remove them after they are built on Docker Hub.

image="$1"

cname="plone-container-$RANDOM-$RANDOM"
cid="$(docker run -d -e PLONE_DEVELOP=src/eea.facetednavigation -e PLONE_ADDONS=eea.facetednavigation -e PLONE_ZCML=eea.facetednavigation-meta --name "$cname" "$image" cat)"
Copy link
Member

Choose a reason for hiding this comment

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

This container will likely be dead by the time thedocker exec below tries to execute and would then fail. Maybe just replace it all with a docker run -i --rm -e .... cat custom.cfg. Plus we would get to drop the trap 😄.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yosifkit

Fixed it. Thank you!

@yosifkit
Copy link
Member

Diff for the record: https://gist.github.com/anonymous/62ca1bed63c16d4b15b5970fe6ed49c7

Test build in progress. 🌮 🎉

@yosifkit
Copy link
Member

Build test of #1434; 744116f (plone):

$ bashbrew build plone:5.0.6
Building bashbrew/cache:4d9ae74eb90dfb044f0105d0d2bab767c293faa430a70ba18ff34c0821378809 (plone:5.0.6)
Tagging plone:5.0.6
Tagging plone:5.0
Tagging plone:5
Tagging plone:latest

$ test/run.sh plone:5.0.6
testing plone:5.0.6
    'utc' [1/7]...passed
    'cve-2014--shellshock' [2/7]...passed
    'no-hard-coded-passwords' [3/7]...passed
    'override-cmd' [4/7]...passed
    'plone-basics' [5/7]....passed
    'plone-addons' [6/7]...passed
    'plone-zeoclient' [7/7]....passed


$ bashbrew build plone:5.0.5
Building bashbrew/cache:9c64701bc8d8a2543a7edd2cc9c6f9569f2cbbe865c4c757e710624c1db061bb (plone:5.0.5)
Tagging plone:5.0.5

$ test/run.sh plone:5.0.5
testing plone:5.0.5
    'utc' [1/7]...passed
    'cve-2014--shellshock' [2/7]...passed
    'no-hard-coded-passwords' [3/7]...passed
    'override-cmd' [4/7]...passed
    'plone-basics' [5/7]....passed
    'plone-addons' [6/7]...passed
    'plone-zeoclient' [7/7]....passed


$ bashbrew build plone:5.0.4
Building bashbrew/cache:1062fa294996dbf1a938228708254c8162d2052af7f1485c713374334c6396ff (plone:5.0.4)
Tagging plone:5.0.4

$ test/run.sh plone:5.0.4
testing plone:5.0.4
    'utc' [1/7]...passed
    'cve-2014--shellshock' [2/7]...passed
    'no-hard-coded-passwords' [3/7]...passed
    'override-cmd' [4/7]...passed
    'plone-basics' [5/7]....passed
    'plone-addons' [6/7]...passed
    'plone-zeoclient' [7/7]....passed


$ bashbrew build plone:5.0.3
Building bashbrew/cache:32303d503b590becce1082a80d6122aee421fba0a7628cb94bde0bfb5b107cbf (plone:5.0.3)
Tagging plone:5.0.3

$ test/run.sh plone:5.0.3
testing plone:5.0.3
    'utc' [1/7]...passed
    'cve-2014--shellshock' [2/7]...passed
    'no-hard-coded-passwords' [3/7]...passed
    'override-cmd' [4/7]...passed
    'plone-basics' [5/7]....passed
    'plone-addons' [6/7]...passed
    'plone-zeoclient' [7/7]....passed


$ bashbrew build plone:5.0.2
Building bashbrew/cache:c8ea2d20ac0d94a2bad46f8f45a3b895b4387f8a3074dd128bc29ab116a7e34c (plone:5.0.2)
Tagging plone:5.0.2

$ test/run.sh plone:5.0.2
testing plone:5.0.2
    'utc' [1/7]...passed
    'cve-2014--shellshock' [2/7]...passed
    'no-hard-coded-passwords' [3/7]...passed
    'override-cmd' [4/7]...passed
    'plone-basics' [5/7]....passed
    'plone-addons' [6/7]...passed
    'plone-zeoclient' [7/7]....passed


$ bashbrew build plone:4.3.11
Building bashbrew/cache:f22d6ee6113b298b44fadc0f8f88b53e936e22fa87f718ebc4650f51e16fa31e (plone:4.3.11)
Tagging plone:4.3.11
Tagging plone:4.3
Tagging plone:4

$ test/run.sh plone:4.3.11
testing plone:4.3.11
    'utc' [1/7]...passed
    'cve-2014--shellshock' [2/7]...passed
    'no-hard-coded-passwords' [3/7]...passed
    'override-cmd' [4/7]...passed
    'plone-basics' [5/7]....passed
    'plone-addons' [6/7]...passed
    'plone-zeoclient' [7/7]....passed


$ bashbrew build plone:4.3.10
Building bashbrew/cache:09f1e276a6a6a9848bce9d78bffb58f0b2f4e03145eafe3f14e1dd37539627dc (plone:4.3.10)
Tagging plone:4.3.10

$ test/run.sh plone:4.3.10
testing plone:4.3.10
    'utc' [1/7]...passed
    'cve-2014--shellshock' [2/7]...passed
    'no-hard-coded-passwords' [3/7]...passed
    'override-cmd' [4/7]...passed
    'plone-basics' [5/7]....passed
    'plone-addons' [6/7]...passed
    'plone-zeoclient' [7/7]....passed


$ bashbrew build plone:4.3.9
Building bashbrew/cache:20ef176dc1525d440ae7465c0672e76cad07d77b4009820e6a64b61f0644aa19 (plone:4.3.9)
Tagging plone:4.3.9

$ test/run.sh plone:4.3.9
testing plone:4.3.9
    'utc' [1/7]...passed
    'cve-2014--shellshock' [2/7]...passed
    'no-hard-coded-passwords' [3/7]...passed
    'override-cmd' [4/7]...passed
    'plone-basics' [5/7]....passed
    'plone-addons' [6/7]...passed
    'plone-zeoclient' [7/7]....passed


$ bashbrew build plone:4.3.8
Building bashbrew/cache:6089bfcb995cb2c422d7f0930e45bfa21a394be6acaeba23eff72f3d29f8b351 (plone:4.3.8)
Tagging plone:4.3.8

$ test/run.sh plone:4.3.8
testing plone:4.3.8
    'utc' [1/7]...passed
    'cve-2014--shellshock' [2/7]...passed
    'no-hard-coded-passwords' [3/7]...passed
    'override-cmd' [4/7]...passed
    'plone-basics' [5/7]....passed
    'plone-addons' [6/7]...passed
    'plone-zeoclient' [7/7]....passed


$ bashbrew build plone:4.3.7
Building bashbrew/cache:396357c23b8e966725585a8f3db19db942e6adb3ac03925ff7635cfccaa4c883 (plone:4.3.7)
Tagging plone:4.3.7

$ test/run.sh plone:4.3.7
testing plone:4.3.7
    'utc' [1/7]...passed
    'cve-2014--shellshock' [2/7]...passed
    'no-hard-coded-passwords' [3/7]...passed
    'override-cmd' [4/7]...passed
    'plone-basics' [5/7]....passed
    'plone-addons' [6/7]...passed
    'plone-zeoclient' [7/7]....passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants