Improve Dockerfile

[zguig52]

• add curl for HTTPS healthchecks (Docker - add curl - wget from alpine cannot implement healthcheck with HTTPS enabled #1961)
• remove forced port binding from CMD to allow config file use(Cannot change listening hosts through config file in docker container due to forced host command #1960)
• remove default TCP port EXPOSE command as it is not really usefull (https://forums.docker.com/t/what-is-the-use-of-expose-in-docker-file/37726)

[pbiering]

@kalsi-avneet - any ETA for review?

[kalsi-avneet]

@kalsi-avneet - any ETA for review?

Sorry for the delay. I had a busy week.

I have reviewed the PR and am waiting for the reply to my request in #1960 (comment) :

Could you share your config file and compose.yaml/docker-compose.yml/docker run command? I was not able to get hosts = localhost:15232 to work. Changing it to hosts = 0.0.0.0:15232 works.

Here is my review:

changes proposed in this PR

1. add curl for HTTPS healthchecks
2. remove forced port binding from CMD to allow config file use
3. remove default TCP port EXPOSE command as it is not really usefull

My review

• I agree with (1) and (3)
• I have some questions/discussion points about (2) as below:

remove forced port binding from CMD to allow config file use

This proposed change would allow the user to allow changing the port inside the docker container by reading the config file.

The current behavior of Radicale's docker is that the user does not need to specify a port for inside the container.
Port/IP for outside the container can be published using the compose.yaml or docker run command. After applying the current change, the user will have to specify the port explicitly. This will be an additional requirement for the user.

My question/comment about localhost was because Radicale defaults to listening on localhost:5232 even when no config is specified ¹, but this would not work inside the container, because 0.0.0.0:5232 would have to be specified in order for docker to listen on that port. Since hosts = localhost:15232 was mentioned in the issue specifically, I was interested to know how that was achieved, because then the user would not have to explicitly specify the IP:port if they want the default.

Additionally, the Dockerfile is also used to publish on Cloudron, so I am concerned that this change might impact them.

@pbiering , would it be possible to add the maintainer of Cloudron packaging as a reviewer too?

Footnotes

1. https://github.com/Kozea/Radicale/blob/a4a39318c38658a8915e2ecd43cada758b0c57e5/DOCUMENTATION.md#L824-L826 ↩

[pbiering]

would it be possible to add the maintainer of Cloudron packaging as a reviewer too?

Name? Potentially you have to mention first and then I can add.

[kalsi-avneet]

would it be possible to add the maintainer of Cloudron packaging as a reviewer too?

Name? Potentially you have to mention first and then I can add.

Oh, I was hoping you might know 😅

Let me see if I can figure out from the documentation or code

[kalsi-avneet]

Let me see if I can figure out from the documentation or code

I could not find maintainer's name in documentation

Radicale/DOCUMENTATION.md

Lines 2683 to 2706 in 4fb1672

	Radicale has been packaged for:
	* [ArchLinux](https://www.archlinux.org/packages/community/any/radicale/) by
	David Runge
	* [Debian](https://packages.debian.org/radicale) by Jonas Smedegaard
	* [Gentoo](https://packages.gentoo.org/packages/www-apps/radicale)
	by René Neumann, Maxim Koltsov and Manuel Rüger
	* [Fedora/EnterpriseLinux](https://src.fedoraproject.org/rpms/radicale) by Jorti
	and Peter Bieringer
	* [Mageia](http://madb.mageia.org/package/show/application/0/name/radicale)
	by Jani Välimaa
	* [OpenBSD](http://openports.se/productivity/radicale) by Sergey Bronnikov,
	Stuart Henderson and Ian Darwin
	* [openSUSE](http://software.opensuse.org/package/Radicale?search_term=radicale)
	by Ákos Szőts and Rueckert
	* [PyPM](http://code.activestate.com/pypm/radicale/)
	* [Slackware](http://schoepfer.info/slackware.xhtml#packages-network) by
	Johannes Schöpfer
	* [Trisquel](http://packages.trisquel.info/search?searchon=names&keywords=radicale)
	* [Ubuntu](http://packages.ubuntu.com/radicale) by the MOTU and Jonas
	Smedegaard
	Radicale is also
	[available on Cloudron](https://cloudron.io/button.html?app=org.radicale.cloudronapp2).

[kalsi-avneet]

@zguig52 ,

Could you revert only this line?

The reason, as I have mentioned in #1962 (comment) is that a docker container created without this line would require the user to explicitly declare hosts = 0.0.0.0:<PORT> in the config file explicitly, even if defaults are not changed.

I am not 100% sure, but I am concerned that this might impact cloudron packaging.

Till it is confirmed, the other 2 changes in the PR can be merged.

[kalsi-avneet]

#1962 (comment)

Here it is. I did not thought about the broad impact for existing deployments

@zguig52 ,

Thank you for reverting the line.

Although there would be no difference functionally, I would like to request a small change : Currently in the diff of this PR, it seems that the line CMD ["--hosts", "0.0.0.0:5232,[::]:5232"] has moved. Could you modify it such that there is no change in this line (since the change has been reverted)

Here is an example to demonstrate my request:

Current diff:

@@ -19,16 +19,14 @@ WORKDIR /app
 
 RUN addgroup -g 1000 radicale \
     && adduser radicale --home /var/lib/radicale --system --uid 1000 --disabled-password -G radicale \
-    && apk add --no-cache ca-certificates openssl
+    && apk add --no-cache ca-certificates openssl curl
 
 COPY --chown=radicale:radicale --from=builder /app/venv /app
 
 # Persistent storage for data
 VOLUME /var/lib/radicale
-# TCP port of Radicale
-EXPOSE 5232
 # Run Radicale
 ENTRYPOINT [ "/app/bin/python", "/app/bin/radicale"]
-CMD ["--hosts", "0.0.0.0:5232,[::]:5232"]
 
 USER radicale
+CMD ["--hosts", "0.0.0.0:5232,[::]:5232"]

Note how this diff seems to contain 3 changes:

1. Install curl
2. Remove EXPOSE 5232
3. Delete the CMD ["--hosts" ... line from below the ENTRYPOINT ... line and place it below the USER radicale line

Requested diff

@@ -19,16 +19,14 @@ WORKDIR /app
 
 RUN addgroup -g 1000 radicale \
     && adduser radicale --home /var/lib/radicale --system --uid 1000 --disabled-password -G radicale \
-    && apk add --no-cache ca-certificates openssl
+    && apk add --no-cache ca-certificates openssl curl
 
 COPY --chown=radicale:radicale --from=builder /app/venv /app
 
 # Persistent storage for data
 VOLUME /var/lib/radicale
-# TCP port of Radicale
-EXPOSE 5232
 # Run Radicale
 ENTRYPOINT [ "/app/bin/python", "/app/bin/radicale"]

Note how this diff only contains 2 changes (no change in the CMD ["--hosts" ... line).

This is no difference in functionality, but the commit would appear more precise.

[zguig52]

Here it is. I did not thought about the broad impact for existing deployments