Feature/add dockerfile and docker compose and add new release workflow#30
Feature/add dockerfile and docker compose and add new release workflow#30aviralgarg05 merged 8 commits intoaviralgarg05:mainfrom ignorant05:feature/Add-Dockerfile-and-docker-compose-and-add-new-release-workflow
Conversation
|
Keep this PR in a mergeable state → Learn moreAll Green is an AI agent that automatically: ✅ Addresses code review comments ✅ Fixes failing CI checks ✅ Resolves merge conflicts |
There was a problem hiding this comment.
Pull request overview
This PR adds Docker support to NexumDB by introducing containerization and automated Docker image publishing. It addresses issue #15 by providing a complete Docker workflow including build configuration, compose orchestration, and CI/CD integration for releasing Docker images to Docker Hub.
Key Changes:
- Added multi-stage Dockerfile for building and running the Rust/Python hybrid application
- Created docker-compose.yaml for simplified local development and deployment
- Added GitHub Actions workflow for automated Docker image releases
- Updated README with Docker usage instructions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| Dockerfile | Multi-stage build configuration compiling Rust binary and setting up Python environment |
| docker-compose.yaml | Service definition for running NexumDB with volume mounts and network configuration |
| README.md | Added documentation for Docker Compose commands (build, run, stop, logs) |
| .github/workflows/docker-release.yml | Automated workflow to build and push Docker images on release creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with: | ||
| username: ${{ secrets.DOCKER_USERNAME }} | ||
| password: ${{ secrets.DOCKER_PASSWORD }} |
There was a problem hiding this comment.
Security concern with Docker Hub password: Using DOCKER_PASSWORD as a secret name is acceptable, but it's recommended to use Docker Hub access tokens instead of passwords for CI/CD workflows. Access tokens provide better security through scoped permissions and easier revocation. Consider updating the documentation to recommend using a Personal Access Token (PAT) for the DOCKER_PASSWORD secret.
|
|
||
| ### Logs | ||
|
|
||
| ```badh |
There was a problem hiding this comment.
Typo in code block language identifier: badh should be bash.
| ```badh | |
| ```bash |
| && rm -rf /var/lib/apt/lists/* | ||
|
|
||
| COPY Cargo.toml Cargo.lock ./ | ||
| COPY . . |
There was a problem hiding this comment.
Inefficient Docker layer caching: Copying the entire context (line 17) immediately after copying Cargo.toml and Cargo.lock (line 16) negates the benefit of dependency caching. The second COPY overwrites the first. Consider restructuring to copy only source files after the dependency files, or remove line 16 entirely if you intend to copy everything at once.
| COPY . . | |
| COPY src ./src |
| - ./config.toml:/app/config.toml | ||
| networks: | ||
| - app-network | ||
|
|
There was a problem hiding this comment.
Missing port exposure configuration: The docker-compose.yaml file doesn't expose any ports for the application. This makes it impossible to connect to the database from outside the container. Consider adding a ports section to map the application's port(s) to the host, e.g.:
ports:
- "8080:8080" # Adjust based on actual application port| ports: | |
| - "8080:8080" # Adjust based on actual application port |
| - name: Set up docker Buildx | ||
| uses: docker/setup-buildx-action@v2 | ||
|
|
||
| - name: Docker-hub account login |
There was a problem hiding this comment.
Inconsistent capitalization in step name: "Docker-hub" should be "Docker Hub" (with capital H and space) for proper branding and consistency with Docker's official naming.
| - name: Docker-hub account login | |
| - name: Docker Hub account login |
| ```badh | ||
| $ docker-compose logs |
There was a problem hiding this comment.
Inconsistent command format: This line uses docker-compose (with hyphen) while the other commands use docker compose (with space). For consistency and to follow modern Docker Compose V2 syntax, use docker compose throughout.
| ```badh | |
| $ docker-compose logs | |
| ```bash | |
| $ docker compose logs |
| RUN apt-get update && apt-get install -y \ | ||
| python3 \ | ||
| python3-venv \ | ||
| libpython3.11 \ |
There was a problem hiding this comment.
Hard-coded Python version creates portability issues: The libpython3.11 dependency is specific to Python 3.11, which may not be available or might differ in future Debian versions. Consider using a more generic approach like python3 and python3-dev packages, or explicitly install Python 3.11 from a specific source if this exact version is required.
| libpython3.11 \ | |
| python3-dev \ |
| - RUST_LOG=info | ||
| volumes: | ||
| - ./data:/app/data | ||
| - ./config.toml:/app/config.toml |
There was a problem hiding this comment.
Volume mount references non-existent file: The docker-compose.yaml mounts ./config.toml:/app/config.toml, but there's no config.toml file in the repository. This will cause the container to fail to start or create an empty directory mount. Either provide a default config.toml file in the repository, make this mount optional, or document that users need to create this file before running docker compose.
| uses: actions/checkout@v3 | ||
|
|
||
| - name: Set up docker Buildx | ||
| uses: docker/setup-buildx-action@v2 | ||
|
|
||
| - name: Docker-hub account login | ||
| uses: docker/login-action@v2 | ||
| with: | ||
| username: ${{ secrets.DOCKER_USERNAME }} | ||
| password: ${{ secrets.DOCKER_PASSWORD }} | ||
|
|
||
| - name: Extract release tag | ||
| id: meta | ||
| run: echo "version=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT | ||
|
|
||
| - name: Build and push Docker image | ||
| uses: docker/build-push-action@v3 |
There was a problem hiding this comment.
Using outdated GitHub Actions versions: The workflow uses @v2 and @v3 versions of actions while newer @v4 versions are available (as seen in ci.yml which uses @v4 for checkout). Consider updating to the latest versions:
actions/checkout@v3→actions/checkout@v4docker/setup-buildx-action@v2→docker/setup-buildx-action@v3docker/login-action@v2→docker/login-action@v3docker/build-push-action@v3→docker/build-push-action@v5
This ensures you have the latest features and security updates.
|
|
||
| ENV PATH="/app/.venv/bin:$PATH" | ||
| ENV VIRTUAL_ENV="/app/.venv" | ||
|
|
There was a problem hiding this comment.
The container currently runs nexum as the default root user (no USER directive is set), so any compromise of the application results in full root privileges inside the container and write access to mounted host volumes. To reduce the blast radius, create a dedicated unprivileged user, adjust ownership/permissions on /app and data paths, and add a USER directive (or user in docker-compose) so the process does not run as root.
| ENV PATH="/app/.venv/bin:$PATH" | |
| ENV VIRTUAL_ENV="/app/.venv" | |
| # Create unprivileged user and group | |
| RUN useradd --system --create-home --home-dir /app --shell /bin/bash nexumuser && \ | |
| chown -R nexumuser:nexumuser /app | |
| ENV PATH="/app/.venv/bin:$PATH" | |
| ENV VIRTUAL_ENV="/app/.venv" | |
| USER nexumuser |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,20 @@ | |||
| version: "3.8" | |||
There was a problem hiding this comment.
The version field in docker-compose.yaml is obsolete. Docker Compose v1.27.0+ (and all v2.x versions) no longer require or use the top-level version field. The Compose Specification considers it optional and the field is ignored.
Consider removing this line:
services:
nexumdb-app:
build:| version: "3.8" |
| ### Run the application | ||
|
|
||
| ```bash | ||
| $ docker compose up |
There was a problem hiding this comment.
Trailing whitespace after the command. Remove the trailing space for consistency with other code blocks.
| $ docker compose up | |
| $ docker compose up |
| networks: | ||
| - app-network | ||
| ports: | ||
| - "8080:8080" | ||
|
|
||
| networks: | ||
| app-network: | ||
| driver: bridge |
There was a problem hiding this comment.
[nitpick] The custom app-network is defined but not necessary for a single-service application. Docker Compose automatically creates a default network for all services. The custom network adds unnecessary complexity without providing any benefit.
Consider removing the networks section entirely unless you plan to add more services that need network isolation:
services:
nexumdb-app:
build:
context: .
dockerfile: Dockerfile
container_name: nexumdb
environment:
- PYO3_USE_ABI3_FORWARD_COMPATIBILITY=1
- RUST_LOG=info
volumes:
- ./data:/app/nexumdb_data
stdin_open: true
tty: true| networks: | |
| - app-network | |
| ports: | |
| - "8080:8080" | |
| networks: | |
| app-network: | |
| driver: bridge | |
| ports: | |
| - "8080:8080" |
There was a problem hiding this comment.
i think that it is a necessity.
There was a problem hiding this comment.
A custom network makes sense if we:
Plan to add more services and explicitly isolate groups of them, or maybe need to reuse a named network across multiple Compose projects (e.g. with external: true), or we want to tweak driver options / attach non-Compose containers manually.
If that’s the plan, I’m totally fine keeping it — but then it might be good to add a short comment in the file explaining why app-network is needed so future contributors don’t think it’s dead config. Otherwise, I’d still recommend dropping the explicit networks section to keep the compose file minimal and easier to maintain.
There was a problem hiding this comment.
Then i'll remove it since it isn't needed now and you want to keep it minimal for easy maintenance, consider adding opening an issue when it needs to be sat, which sounds like a good plan for now.
There was a problem hiding this comment.
Then i'll remove it since it isn't needed now and you want to keep it minimal for easy maintenance, consider adding opening an issue when it needs to be sat, which sounds like a good plan for now.
Sure will do that
| COPY src ./src | ||
|
|
There was a problem hiding this comment.
The Dockerfile copies source from a non-existent src directory. Based on the workspace structure in Cargo.toml, the project uses workspace members (nexum_core, nexum_cli, tests), not a root-level src directory.
This line should copy all workspace members:
COPY nexum_cli ./nexum_cli
COPY nexum_core ./nexum_core
COPY tests ./tests| COPY src ./src | |
| COPY nexum_core ./nexum_core | |
| COPY nexum_cli ./nexum_cli | |
| COPY tests ./tests |
There was a problem hiding this comment.
bruh, u were the one who suggested it. yup AI is an AI afterall.
| ${{ secrets.DOCKER_USERNAME }}/nexumdb:latest | ||
| ${{ secrets.DOCKER_USERNAME }}/nexumdb:${{ steps.meta.outputs.version }} | ||
| ${{ secrets.DOCKER_USERNAME }}/nexumdb:${{ github.sha }} |
There was a problem hiding this comment.
Inconsistent secret names used in the workflow. Line 20 uses DOCKERHUB_USERNAME, line 34 uses DOCKER_USERNAME, and lines 37-38 use DOCKERHUB_USERNAME.
All references should use the same secret name. Consider using DOCKERHUB_USERNAME consistently throughout:
tags: |
${{ secrets.DOCKERHUB_USERNAME }}/nexumdb:latest
${{ secrets.DOCKERHUB_USERNAME }}/nexumdb:${{ steps.meta.outputs.version }}
${{ secrets.DOCKERHUB_USERNAME }}/nexumdb:${{ github.sha }}| ${{ secrets.DOCKER_USERNAME }}/nexumdb:latest | |
| ${{ secrets.DOCKER_USERNAME }}/nexumdb:${{ steps.meta.outputs.version }} | |
| ${{ secrets.DOCKER_USERNAME }}/nexumdb:${{ github.sha }} | |
| ${{ secrets.DOCKERHUB_USERNAME }}/nexumdb:latest | |
| ${{ secrets.DOCKERHUB_USERNAME }}/nexumdb:${{ steps.meta.outputs.version }} | |
| ${{ secrets.DOCKERHUB_USERNAME }}/nexumdb:${{ github.sha }} |
| ### Stop the application | ||
|
|
||
| ```bash | ||
| $ docker compose down |
There was a problem hiding this comment.
Trailing whitespace after the command. Remove the trailing space for consistency with other code blocks.
| $ docker compose down | |
| $ docker compose down |
| RUN python3 -m venv .venv && \ | ||
| . .venv/bin/activate && \ | ||
| pip install --no-cache-dir -r nexum_ai/requirements.txt | ||
|
|
||
| RUN useradd --system --create-home --home-dir /app --shell /bin/bash nexumuser && \ | ||
| chown -R nexumuser:nexumuser /app | ||
|
|
||
| ENV PATH="/app/.venv/bin:$PATH" | ||
| ENV VIRTUAL_ENV="/app/.venv" | ||
|
|
||
| USER nexumuser | ||
|
|
There was a problem hiding this comment.
The Python dependencies are installed as root before switching to nexumuser. While this is functionally correct, it's a security best practice to create the user earlier and run pip install as the non-root user.
Consider reordering:
RUN useradd --system --create-home --home-dir /app --shell /bin/bash nexumuser && \
chown -R nexumuser:nexumuser /app
USER nexumuser
RUN python3 -m venv .venv && \
. .venv/bin/activate && \
pip install --no-cache-dir -r nexum_ai/requirements.txt| RUN python3 -m venv .venv && \ | |
| . .venv/bin/activate && \ | |
| pip install --no-cache-dir -r nexum_ai/requirements.txt | |
| RUN useradd --system --create-home --home-dir /app --shell /bin/bash nexumuser && \ | |
| chown -R nexumuser:nexumuser /app | |
| ENV PATH="/app/.venv/bin:$PATH" | |
| ENV VIRTUAL_ENV="/app/.venv" | |
| USER nexumuser | |
| RUN useradd --system --create-home --home-dir /app --shell /bin/bash nexumuser && \ | |
| chown -R nexumuser:nexumuser /app | |
| USER nexumuser | |
| RUN python3 -m venv .venv && \ | |
| . .venv/bin/activate && \ | |
| pip install --no-cache-dir -r nexum_ai/requirements.txt | |
| ENV PATH="/app/.venv/bin:$PATH" | |
| ENV VIRTUAL_ENV="/app/.venv" |
| COPY src ./src | ||
|
|
||
| ENV PYO3_USE_ABI3_FORWARD_COMPATIBILITY=1 | ||
|
|
||
| RUN cargo build --release | ||
|
|
There was a problem hiding this comment.
[nitpick] The Dockerfile doesn't leverage Docker layer caching for Rust dependencies. Copying Cargo.toml and Cargo.lock before the source code is good, but you should also create dummy source files and build dependencies first. This way, dependency builds are cached and only rebuild when Cargo.toml or Cargo.lock change.
Consider adding a dependency caching layer:
COPY Cargo.toml Cargo.lock ./
COPY nexum_cli/Cargo.toml ./nexum_cli/
COPY nexum_core/Cargo.toml ./nexum_core/
COPY tests/Cargo.toml ./tests/
# Create dummy source files to build dependencies
RUN mkdir -p nexum_cli/src nexum_core/src tests/src && \
echo "fn main() {}" > nexum_cli/src/main.rs && \
echo "pub fn dummy() {}" > nexum_core/src/lib.rs && \
echo "pub fn dummy() {}" > tests/src/lib.rs
# Build dependencies (this layer will be cached)
RUN cargo build --release
# Now copy actual source and rebuild (only your code, deps are cached)
COPY nexum_cli ./nexum_cli
COPY nexum_core ./nexum_core
COPY tests ./tests
RUN touch nexum_cli/src/main.rs && cargo build --release| COPY src ./src | |
| ENV PYO3_USE_ABI3_FORWARD_COMPATIBILITY=1 | |
| RUN cargo build --release | |
| # Create dummy src/main.rs to build dependencies only | |
| RUN mkdir -p src && echo "fn main() {}" > src/main.rs | |
| ENV PYO3_USE_ABI3_FORWARD_COMPATIBILITY=1 | |
| # Build dependencies (this layer will be cached unless Cargo.toml/Cargo.lock change) | |
| RUN cargo build --release | |
| # Now copy actual source code and rebuild | |
| COPY src ./src | |
| RUN touch src/main.rs && cargo build --release |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
docker-compose.yaml:16
- The nexum application is an interactive CLI REPL (Read-Eval-Print Loop) that reads from stdin, not a network server. Exposing port 8080 doesn't serve any purpose since the application doesn't listen on any ports.
To run an interactive CLI application in Docker, you need to use the -it flags. Consider one of these approaches:
- Remove the ports mapping if running interactively:
services:
nexumdb-app:
build:
context: .
dockerfile: Dockerfile
container_name: nexumdb
environment:
- PYO3_USE_ABI3_FORWARD_COMPATIBILITY=1
- RUST_LOG=info
volumes:
- ./data:/app/data
networks:
- app-network
stdin_open: true
tty: trueThen run with: docker compose run --rm nexumdb-app
- Or add documentation explaining that users should connect with
docker exec -it nexumdb /bin/bashand runnexummanually.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ```bash | ||
| $ docker compose up | ||
| ``` | ||
|
|
There was a problem hiding this comment.
The Docker instructions don't explain that nexum is an interactive CLI application. Users following these instructions will find the container starts but they can't interact with it.
Add a note explaining how to connect to the interactive shell. For example:
### Run the application
```bash
$ docker compose up -d
$ docker exec -it nexumdb nexumOr run it interactively:
$ docker compose run --rm nexumdb-app
This will help users understand how to actually use the Dockerized application.
```suggestion
> **Note:** NexumDB is an interactive CLI application. After starting the container, you need to connect to the CLI to interact with the database.
>
> To open an interactive shell in the running container:
>
> ```bash
> $ docker exec -it nexumdb nexum
> ```
>
> Or run it interactively (without starting in detached mode):
>
> ```bash
> $ docker compose run --rm nexumdb-app
> ```
| RUN mkdir -p src && echo "fn main() {}" > src/main.rs | ||
|
|
||
| ENV PYO3_USE_ABI3_FORWARD_COMPATIBILITY=1 | ||
|
|
||
| RUN cargo build --release |
There was a problem hiding this comment.
The Dockerfile creates a dummy src/main.rs in the root directory for dependency caching, but this project structure has the actual binary in nexum_cli/src/main.rs (as shown in nexum_cli/Cargo.toml). This dummy file will not match the workspace structure and cargo build --release on line 27 will fail.
Instead, you should create dummy main.rs files in the correct locations:
RUN mkdir -p nexum_cli/src && echo "fn main() {}" > nexum_cli/src/main.rsAdditionally, you'll need dummy lib.rs files for the library crates:
RUN mkdir -p nexum_core/src && echo "" > nexum_core/src/lib.rs
RUN mkdir -p tests/src && echo "" > tests/src/lib.rs| COPY nexum_cli ./nexum_cli | ||
| COPY tests ./tests | ||
|
|
||
| RUN touch src/main.rs && cargo build --release |
There was a problem hiding this comment.
After copying the real source code, line 33 should also rebuild the workspace binaries. However, since the workspace doesn't have a src/main.rs in the root, the touch src/main.rs will have no effect.
Change this to touch the actual binary location:
RUN touch nexum_cli/src/main.rs && cargo build --release| RUN touch src/main.rs && cargo build --release | |
| RUN touch nexum_cli/src/main.rs && cargo build --release |
| ```bash | ||
| $ docker compose up | ||
| ``` | ||
|
|
There was a problem hiding this comment.
This line has trailing whitespace. Remove it for cleaner formatting.
| RUN cargo build --release | ||
|
|
||
| COPY nexum_core ./nexum_core | ||
| COPY nexum_cli ./nexum_cli | ||
| COPY tests ./tests | ||
|
|
||
| RUN touch src/main.rs && cargo build --release |
There was a problem hiding this comment.
[nitpick] The Dockerfile copies the tests/ directory but it's only needed for building, not for the final runtime image. The tests crate is being built in the builder stage but isn't used in the runtime.
If tests aren't needed in the Docker build at all, consider excluding them from the workspace build by using --workspace --exclude tests:
RUN cargo build --release --workspace --exclude testsThis will speed up the build and reduce the builder image size.
| RUN cargo build --release | |
| COPY nexum_core ./nexum_core | |
| COPY nexum_cli ./nexum_cli | |
| COPY tests ./tests | |
| RUN touch src/main.rs && cargo build --release | |
| RUN cargo build --release --workspace --exclude tests | |
| COPY nexum_core ./nexum_core | |
| COPY nexum_cli ./nexum_cli | |
| COPY tests ./tests | |
| RUN touch src/main.rs && cargo build --release --workspace --exclude tests |
There was a problem hiding this comment.
I do not agree with this, at least the first half, i mean it's better to build it all and run the tests in the building stage then exclude them in the rebuilding before the runtime stage, so that i makes sure everything works fine.
There was a problem hiding this comment.
Already made sure of building the whole app and run the tests first, then proceeded with building it while excluding them afterwards.
| USER nexumuser | ||
|
|
||
| RUN python3 -m venv .venv && \ | ||
| . .venv/bin/activate && \ |
There was a problem hiding this comment.
Installing PyTorch and ML dependencies without specifying CPU-only versions will default to downloading CUDA-enabled packages, which are significantly larger (~2-3GB extra). For a general Docker image, consider using CPU-only PyTorch to reduce image size:
RUN python3 -m venv .venv && \
. .venv/bin/activate && \
pip install --no-cache-dir torch --index-url https://download.pytorch.org/whl/cpu && \
pip install --no-cache-dir -r nexum_ai/requirements.txtThis will make the image much smaller and build faster, which likely addresses the storage issue mentioned in the PR description.
| . .venv/bin/activate && \ | |
| . .venv/bin/activate && \ | |
| pip install --no-cache-dir torch --index-url https://download.pytorch.org/whl/cpu && \ |
| ### Logs | ||
|
|
||
| ```bash | ||
| $ docker-compose logs |
There was a problem hiding this comment.
The README uses inconsistent Docker Compose commands. Line 85 uses docker-compose (with a hyphen), while lines 67, 73, and 79 use docker compose (without a hyphen).
The modern Docker Compose V2 uses docker compose (space-separated). For consistency, change line 85 to:
$ docker compose logs| $ docker-compose logs | |
| $ docker compose logs |
There was a problem hiding this comment.
Both runs fine for me.
There was a problem hiding this comment.
Both runs fine for me.
Yea, you can skip this
| run: echo "version=${GITHUB_REF#refs/tags/}" >> $GITHUB_OUTPUT | ||
|
|
||
| - name: Build and push Docker image | ||
| uses: docker/build-push-action@v5 |
There was a problem hiding this comment.
The workflow uses docker/build-push-action@v5, but this action is now at v6 (released in 2024). Consider updating to the latest version for improved features, bug fixes, and security updates:
uses: docker/build-push-action@v6| uses: docker/build-push-action@v5 | |
| uses: docker/build-push-action@v6 |
| $ docker compose up | ||
| ``` | ||
|
|
||
| ### Stop the application |
There was a problem hiding this comment.
This line has trailing whitespace. Remove it for cleaner formatting.
| ### Stop the application | |
| ### Stop the application |
…the dockerfile, docker-release.yml and the docker-compose.yaml files
aviralgarg05
left a comment
There was a problem hiding this comment.
Looks good,Thanks for the contribution
Summary
Checklist
Additional Notes