Skip to content

Conversation

@kevinruffus
Copy link

  1. Added example env file to allow for assignable info and creds
  2. Updated docker-compose files to reflect env file addition and use updated version options
  3. Now copies example json and ini files for non mlapi version to correct location and enables persistence
  4. Defaults to using docker managed persistent volumes
  5. Uses current MariaDB image and adds basic healthcheck function
  6. Updates Zoneminder to release 1.36.35
  7. Updates base image to Debian 13-slim
  8. Bumps PHP ver to 8.4
  9. Added a few php and apache2 security settings
  10. Added Nano to the image because not being able to edit files while attached to the container was absolutely obnoxious.

After or before building you'd need to update the image listed in the docker-compose files to pull from your repo to match the release that's created.

@kevinruffus
Copy link
Author

Hold on putting this through. Checking something that may be an unintended side effect of the updates.

…tain functions, including manual deletion through the events page.
@kevinruffus
Copy link
Author

Removed problematic security settings from php.ini and security.conf
Issue now resolved.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modernizes the Docker setup for Zoneminder by updating to Debian 13, PHP 8.4, and the latest MariaDB, while transitioning from hardcoded credentials to environment variable configuration. The changes include security hardening, improved volume management, and the removal of forced credentials in favor of a configurable .env file approach.

Key Changes:

  • Updates base image to Debian 13-slim and bumps PHP to version 8.4
  • Replaces hardcoded database credentials with environment variables sourced from .env file
  • Migrates to Docker-managed persistent volumes with healthcheck for MariaDB
  • Adds Apache security configurations (ServerTokens, ServerSignature, X-Content-Type-Options headers)

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
Dockerfile Updates base image to Debian 13-slim, adds nano for container editing, and copies default ZMES configuration files to /etc/zm
entrypoint.sh Updates PHP path to 8.4 and adds Apache security hardening through security.conf modifications
docker-compose.yml Migrates to compose version 3.9, implements environment variable configuration, adds MariaDB healthcheck, and switches to Docker-managed volumes
docker-compose-mlapi.yml Applies same modernization as docker-compose.yml with additional mlapi service configuration
.env.example Introduces environment variable template for configurable database credentials, timezone, and port mappings
zmeventnotification.EXAMPLE.ini Removed - configuration files no longer included as examples (replaced by approach expecting user-provided configs)
secrets.EXAMPLE.ini Removed - configuration files no longer included as examples (replaced by approach expecting user-provided configs)
objectconfig.EXAMPLE.ini Removed - configuration files no longer included as examples (replaced by approach expecting user-provided configs)
es_rules.EXAMPLE.json Removed - configuration files no longer included as examples (replaced by approach expecting user-provided configs)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Dockerfile Outdated
Comment on lines 69 to 73
# Copy default files for ZMES
COPY es_rules.json /etc/zm/
COPY secrets.ini /etc/zm/
COPY zmeventnotification.ini /etc/zm/
COPY objectconfig.ini /etc/zm/
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The Dockerfile attempts to copy configuration files (es_rules.json, secrets.ini, zmeventnotification.ini, objectconfig.ini) that do not exist in the repository. The EXAMPLE versions of these files were deleted in this PR but non-EXAMPLE versions were not added. This will cause the Docker build to fail.

Either:

  1. Add the actual non-EXAMPLE versions of these files to the repository, or
  2. Remove these COPY commands and rely on the user to mount these files as volumes (as mentioned in the README and the docker-compose-mlapi.yml file)
Suggested change
# Copy default files for ZMES
COPY es_rules.json /etc/zm/
COPY secrets.ini /etc/zm/
COPY zmeventnotification.ini /etc/zm/
COPY objectconfig.ini /etc/zm/
# ZMES configuration files should be mounted as volumes by the user (see README and docker-compose-mlapi.yml)

Copilot uses AI. Check for mistakes.

echo "Setting PHP timezone"
sed -i "s|;date\.timezone =.*|date.timezone = ${TZ}|" /etc/php/8.2/apache2/php.ini
sed -i "s|;date\.timezone =.*|date.timezone = ${TZ}|" /etc/php/8.4/apache2/php.ini
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The entrypoint script references PHP 8.4 (/etc/php/8.4/apache2/php.ini), but there's no guarantee that PHP 8.4 will be available in the Debian base image. Additionally, the PR description mentions "Bumps PHP ver to 8.4" but doesn't show any explicit installation of PHP 8.4.

The Dockerfile installs php and php-mysql without specifying a version, which will install whatever version is available in the Debian repository. For Debian 12, this would be PHP 8.2. You need to either:

  1. Explicitly install PHP 8.4 from a third-party repository (like Sury), or
  2. Update this path to match the PHP version that will actually be installed
Suggested change
sed -i "s|;date\.timezone =.*|date.timezone = ${TZ}|" /etc/php/8.4/apache2/php.ini
sed -i "s|;date\.timezone =.*|date.timezone = ${TZ}|" /etc/php/8.2/apache2/php.ini

Copilot uses AI. Check for mistakes.
restart: unless-stopped
shm_size: ${SHM_SIZE}
depends_on:
- mariadb
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The depends_on configuration only ensures that the mariadb container starts before the zm container, but does not wait for the database to be ready. Given that a healthcheck is defined for the mariadb service (lines 25-31), you should use the long-form depends_on syntax to wait for the health condition:

depends_on:
  mariadb:
    condition: service_healthy

This ensures the zm container waits for the database to be fully initialized before starting.

Suggested change
- mariadb
mariadb:
condition: service_healthy

Copilot uses AI. Check for mistakes.
Comment on lines 52 to 53
- ./mlapiconfig.ini:/etc/mlapiconfig.ini
- ./secrets.ini:/etc/secrets.ini
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The mlapi service mounts ./mlapiconfig.ini and ./secrets.ini from the host filesystem, but these files were deleted in this PR (they were EXAMPLE files). Either:

  1. Users need to create these files before running docker-compose (should be documented), or
  2. These files should be included in the repository (without the EXAMPLE prefix), or
  3. The Dockerfile should include default versions of these files that can be overridden by volume mounts

Currently, attempting to run this docker-compose file will fail because these files don't exist.

Copilot uses AI. Check for mistakes.
- video:/var/cache/zoneminder
- zmlogs:/var/log/zm
- weblogs:/var/log/apache2
- zmconfig:/etc/zm
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The zmconfig volume is mounted at /etc/zm, which will initially be empty. However, the Dockerfile copies configuration files to /etc/zm/ (lines 69-73), and the entrypoint script expects certain files to exist at startup. With an empty volume mounted here, the configuration files from the image will be hidden, potentially causing the container to fail at startup.

Consider either:

  1. Removing this volume mount and documenting that users should exec into the container to modify configs, or
  2. Creating an init script that copies default configs from another location to /etc/zm if it's empty on first run, or
  3. Documenting that this configuration approach requires users to pre-populate this volume with configuration files
Suggested change
- zmconfig:/etc/zm

Copilot uses AI. Check for mistakes.
Comment on lines +77 to +78
- weblogs:/var/log/apache2
- zmconfig:/etc/zm
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The zmconfig volume is mounted at /etc/zm, which will initially be empty. However, the Dockerfile copies configuration files to /etc/zm/ (lines 69-73), and the entrypoint script expects certain files to exist at startup. With an empty volume mounted here, the configuration files from the image will be hidden, potentially causing the container to fail at startup.

Consider either:

  1. Removing this volume mount and documenting that users should exec into the container to modify configs, or
  2. Creating an init script that copies default configs from another location to /etc/zm if it's empty on first run, or
  3. Documenting that this configuration approach requires users to pre-populate this volume with configuration files
Suggested change
- weblogs:/var/log/apache2
- zmconfig:/etc/zm
- weblogs:/var/log/apache2

Copilot uses AI. Check for mistakes.
restart: unless-stopped
shm_size: ${SHM_SIZE}
depends_on:
- mariadb
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

The depends_on configuration only ensures that the mariadb container starts before the zm container, but does not wait for the database to be ready. Given that a healthcheck is defined for the mariadb service (lines 25-31), you should use the long-form depends_on syntax to wait for the health condition:

depends_on:
  mariadb:
    condition: service_healthy

This ensures the zm container waits for the database to be fully initialized before starting.

Suggested change
- mariadb
mariadb:
condition: service_healthy

Copilot uses AI. Check for mistakes.
tmpfs:
- /run
- /tmp
- /tmp
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

There is trailing whitespace at the end of this line. While this doesn't affect functionality, it's a best practice to remove trailing whitespace for cleaner code.

Suggested change
- /tmp
- /tmp

Copilot uses AI. Check for mistakes.
- /tmp
environment:
- LOG_DEBUG=true
- /tmp
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

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

There is trailing whitespace at the end of this line. While this doesn't affect functionality, it's a best practice to remove trailing whitespace for cleaner code.

Suggested change
- /tmp
- /tmp

Copilot uses AI. Check for mistakes.
@jantman
Copy link
Owner

jantman commented Nov 15, 2025

@kevinruffus thanks so much for this. I gave a try at using Copilot's code review... full disclosure I've never tried that for a PR from someone else before. The one thing I'm confused about is the removal of the EXAMPLE files... I'm not sure I follow that, and I agree with copilot that it seems like it should cause the Docker build to fail?

@kevinruffus
Copy link
Author

kevinruffus commented Nov 15, 2025

@jantman They actually weren't removed, but instead just renamed and dumped in place, as they would have no real effect unless the option was enabled in the settings through the UI. That being said, I think I have a better idea. A simple ENV variable to "enable" ES, which then runs checks for the files with EXAMPLE in the name, and renames them if they exist. If they don't, it moves on, thus allowing persistence. Simple enough to do. I'll get some of the other things cleaned up as well.

@kevinruffus
Copy link
Author

kevinruffus commented Nov 15, 2025

@jantman Oh! The mlapi ini files. That was a mistake. Thank you for pointing it out.

…ogic for it, fixed mlapi filename references.
@Kasslim
Copy link

Kasslim commented Nov 17, 2025

I did a quick read-through of the diffs and found nothing crazy in the process (keep in mind I'm new at containerization however).

I do want to mention that the following files have diffs on every single line:

  • es_rules.EXAMPLE.json → es_rules_EXAMPLE.json
  • zmeventnotification.EXAMPLE.ini → zmeventnotification_EXAMPLE.ini

I checked and it's because Windows line endings were added (\r\n as opposed to \n). I don't know which line endings are normal for this project, but would generally expect only \n. Git can be configured to fix these automatically when making commits.
I also spotted some erroneous whitespace at the end of several lines. These could probably be removed automatically by your code-editor depending on which one you use (e.g. VSCode).

Can you remediate these (minor) issues? It makes viewing diffs easier and helps keep everything consistent.

Otherwise nice looking changes! I figured I'd review as part of my learning-journey.

@kevinruffus
Copy link
Author

@Kasslim
Thank you for pointing out the end of line issue.
VS Code on Windows defaults to crlf, and it's the second time I've had to fix that now. It must have reset them when I renamed the files the last time. I assume it's actually removing the original files then recreating the "new" renamed file using whatever settings are in Code vs what was in the original file.
This time I set it to all profiles and went through and corrected the affected files.

As to the file names, I went back and forth on what to do with them, ultimately deciding on "enabling" them via an env variable and a bit of logic in the entryfile. I had originally moved them as part of the default setup but realized there wasn't any reason to do so if event notification wasn't going to be used. It looks like I just typed a _ instead of a . when I renamed them the last time.

It's been a while since I've written anything but PowerShell scripts, so I'm a bit rusty myself, and this is the first time I've actually built a container. I've helped with the bash scripting in the entryfile and dockerfile before, but this is my first actual complete build.

@Kasslim
Copy link

Kasslim commented Nov 21, 2025

Happy to help!

I'm excited to try out this repo when I have a little bit of time, perhaps this weekend. I hope to set it up for long term use but the instructions seemed pretty high level (for me), so perhaps I'll write a little guide with commands and explanation as I go. If you already happen to have notes that might boost me along those would be very welcome, but if not no worries. (I also don't know how you'd even share them, since it's off topic for this PR.)

Either way, thanks jantman for setting up the repo and you for working to improve it!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants