Refactor 2025 #9
Conversation
…improved package installation
…es, and multi-architecture support
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
…rectory setup, and set working directory for Doxygen
…tallation and user setup, and list packages alphabetically.
…Dockerfile and improving cache management for image builds
…ure support and add Dockerfile hash generation step in the publish workflow
…lti-distro support and simplify base image selection in Dockerfile
…utions and handle unsupported distros
…ions handling and streamline container execution
… error handling and silent execution
…ed usage examples and permission handling guidance
…t to seem like an official repo
…ity maintenance and issue reporting
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates the Docker infrastructure by unifying separate Alpine and Debian Dockerfiles into a single multi-distro Dockerfile with a smart entrypoint script. The changes modernize the build process, improve permission handling, and significantly enhance the documentation.
Key changes:
- Unified Dockerfile architecture supporting both Alpine and Debian base images through build arguments
- Added comprehensive entrypoint script with PUID/PGID support for permission management
- Consolidated CI/CD workflow using matrix strategy to build both distros efficiently
- Complete README overhaul with detailed usage examples, troubleshooting, and integration guides
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| entrypoint.sh | New entrypoint script handling permissions, user/group ID mapping, and command routing |
| Dockerfile | Unified multi-distro Dockerfile replacing separate Alpine and Debian files |
| Dockerfile.alpine | Removed - replaced by unified Dockerfile |
| Dockerfile.debian | Removed - replaced by unified Dockerfile |
| .github/workflows/docker-publish.yml | Refactored to use matrix strategy, improved caching, and PR-specific tagging |
| README.md | Complete documentation rewrite with comprehensive usage examples and troubleshooting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Check for version command (run silently) | ||
| if [ "$1" = "doxygen" ] && [ "$2" = "-v" ]; then | ||
| exec doxygen -v | ||
| exit 0 |
There was a problem hiding this comment.
The exit 0 after exec is unreachable code. When exec succeeds, it replaces the current process and never returns. This line should be removed.
| exit 0 |
| # Special handling for help command | ||
| if [ "$1" = "doxygen" ] && [ "$2" = "--help" ]; then | ||
| exec doxygen --help | ||
| exit 0 |
There was a problem hiding this comment.
The exit 0 after exec is unreachable code. When exec succeeds, it replaces the current process and never returns. This line should be removed.
| exit 0 |
| ARG DEBIAN_VERSION=stable-slim | ||
|
|
||
| # First set a default image | ||
| FROM ${DISTRO}:${DISTRO_VERSION} |
There was a problem hiding this comment.
When DISTRO=debian, the image will use debian:latest instead of debian:stable-slim because DISTRO_VERSION defaults to latest. The FROM statement should be conditional: use ${DISTRO}:${DISTRO_VERSION} for Alpine and debian:${DEBIAN_VERSION} for Debian. Consider using a multi-stage build or shell logic to select the appropriate base image.
| type=gha,scope=${{ matrix.distro }}-${{ steps.hash.outputs.hash }} | ||
| type=registry,ref=ghcr.io/kingpin/${{ github.event.repository.name }}:${{ matrix.distro }}-cache | ||
| cache-to: | | ||
| type=gha,scope=${{ matrix.distro }},mode=max |
There was a problem hiding this comment.
The cache scope references a hash that is computed but then not used consistently. Line 94 uses scope=${{ matrix.distro }}-${{ steps.hash.outputs.hash }} but line 97 uses scope=${{ matrix.distro }} without the hash. This mismatch means the cache-from won't find the cache created by cache-to, resulting in ineffective caching.
| type=gha,scope=${{ matrix.distro }},mode=max | |
| type=gha,scope=${{ matrix.distro }}-${{ steps.hash.outputs.hash }},mode=max |
| file: ./Dockerfile | ||
| build-args: | | ||
| DISTRO=${{ matrix.distro }} | ||
| ${{ matrix.distro == 'alpine' && format('DISTRO_VERSION={0}', matrix.distro_version) || format('DEBIAN_VERSION={0}', matrix.debian_version) }} |
There was a problem hiding this comment.
The conditional build-arg logic is fragile and will cause issues for Debian builds. When DISTRO=debian, this sets DEBIAN_VERSION=stable-slim but the Dockerfile's FROM statement uses ${DISTRO}:${DISTRO_VERSION}, which would result in debian:latest (the default DISTRO_VERSION value). This needs to properly set both DISTRO and the appropriate version variable.
No description provided.