Refactoring Docker Entrypoint needed?

Development
1

Hey,
I have a few questions about the Docker entrypoint of XWiki. I do not understand why some things are done that way or how to make sense of them. In my opinion, this can be improved by refactoring and better documentation.

  • The main function at the end of the file decides to run first_start or other_start based on the existence of /usr/local/tomcat/webapps/\$CONTEXT_PATH/.first_start_completed. As it can be seen in the Dockerfile, a Docker volume is only declared for the path /usr/local/xwiki. Therefore, if I am not mistaken, that file is never preserved and the code path for other_start is obsolete.
  • In line 153 and 155, the keys environment.permanentDirectory and openoffice.autoStart of xwiki.properties are modified. This piece of code does not run conditionally and the new values are constant (not derived from runtime variables). Why is this not the default option in the config files itself? This could be changed upstream or in the making of the Docker image.
  • Similar question for line 130: When the context path is set to ROOT by default, why must the script explicitely modify xwiki.webapppath in xwiki.cfg? I think most users deploy XWiki in the ROOT context because this is default behaviour. Is it possible to make this the default? How is it enough to copy the web app when the context is changed, but the default case needs special behaviour?
  • In xwiki_replace and safesed, the output file of sed is named “[filename].old”. I think this is very confusing because it is actually the new, modified file that is used to overwrite the original config.
  • Is there anything speaking against using shell variables for e.g. the path to the web app /usr/local/tomcat/webapps/\$CONTEXT_PATH? This would reduce duplicated code.
  • Functions xwiki_replace, xwiki_set_cfg, xwiki_set_properties, file_env and saveConfigurationFile lack a simple comment about what this does imo.
  • I think line 172 would also benefit from a clearer, explainatory comment.
  • I think the comment in line 188 for the command execution is very misleading. There is no if-else case like the comment implies. This line is always run at the end of the script and the previous two if-statements change the command using set

Curious to hear your thoughts on this. I would be wiling to open a smallish PR to refactor, if needed.

Best Regards,
Mali

2

I would like if the lines changing the XWiki config files could be omitted all together. This would allow for read-only mounts when overwriting the main config files. I already saw on GitHub that someone else has the same problem and he proposed placing the configs in the permanent directory again, which is legacy behaviour (files still copied from there for backward compatibility). AFAIK, read-only mounts in Docker containers are an industry standard for production due to security and reproducibility. For example, ConfigMaps in Kubernetes are always mounted as read-only by design.

3

Thx for the review, much appreciated. I’ll try to reply point by point over the coming days.

4

You’re correct that if the container is removed and recreated then the .first_start_completed file will not be there and the configuration will be re-done based on the various environment variables. However, the configured files won’t be copied to /usr/local/xwiki/data because of the IF at xwiki-docker/template/xwiki/docker-entrypoint.sh at cdae3071f5aca70660cfab91fba1385b07b183c7 · xwiki/xwiki-docker · GitHub (called from xwiki-docker/template/xwiki/docker-entrypoint.sh at cdae3071f5aca70660cfab91fba1385b07b183c7 · xwiki/xwiki-docker · GitHub). So indeed, there’ll be a mismatch between the files in usr/local/tomcat/webapps/\$CONTEXT_PATH/WEB-INF and the ones in /usr/local/xwiki/data, until the next restart, when the files from /usr/local/xwiki/data will be copied over.

I think we could decide to drop the .first_start_completed file altogether and merge the first_start and other_start functions, and just decide whether to copy or configure the different files based on their existence or not inside /usr/local/xwiki/data:

  • if they exist, copy them inside the container
  • if they don’t exist, configure the container and save the files in /usr/local/xwiki/data

That said, we now recommend to not map the 3 configuration files and to map them individually if you need to modify them only. See the notes under GitHub - xwiki/xwiki-docker: Dockerfile to build and run XWiki on docker

Thus, if we didn’t want to support backward-compatibility, we would not even copy these files and simply let the user map them to configure them.

Will answer other points later.

5

I don’t quite understand this old behaviour.
The files are either

  1. mounted in a different location to then be copied over (saveConfigurationFile)
  2. Or the configs are “synchronized” from the old mountpoint to the image. Isn’t this the same? Anyways, the other_start function is never run at the moment. Assuming the .first_start_completed file was preserved, I saw nobody report an issue that the restoreConfigurationFile / other_start function was not run. What is holding back from not even copying these files? The new method to map the files directly is not missing out on features, if I am correct. And this is no in-place upgrade or something that would break a XWiki snippet, system or extension. It just affects old deployments of XWiki that are not migrated.
6

I believe this is not correct. I’ve put some echo in it and then:

  • Started the container a first time
  • Stopped it
  • Restarted the container

I could see my echo printed so it’s definitely executed.

7

I do wonder how this is possible due to the check for .first_start_completed. Are you manually creating/saving that file? I do not see by logic how the other_start function could be called.

8

I’m not creating any file manually. The logic is:

  1. The 1st time you start the container, the following is executed:
     if [[ ! -f /usr/local/tomcat/webapps/$CONTEXT_PATH/.first_start_completed ]]; then
   first_start
    This creates the /usr/local/tomcat/webapps/$CONTEXT_PATH/.first_start_completed file.
  2. The 2nd time you start the container (after having stopp it), the following is executed:
     else
   other_starts
 fi
    And this calls other_starts…
9

How are you starting/stopping the container? I understand how the docker-entrypoint.sh intends to choose between first_start and other_starts. But the thing is that the file under /usr/local/tomcat is not preserved when the container is recreated because the volume is declared for /usr/local/xwiki only.