Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Developer docker setup #683

Merged
merged 7 commits into from
Jan 28, 2025
Merged

Developer docker setup #683

merged 7 commits into from
Jan 28, 2025

Conversation

JhumanJ
Copy link
Owner

@JhumanJ JhumanJ commented Jan 28, 2025

Summary by CodeRabbit

  • New Features

    • Added comprehensive Docker-based development environment with multi-service setup.
    • Introduced new Docker configuration files for development and production.
    • Enhanced Nginx configuration for improved API and websocket handling.
  • Documentation

    • Updated contribution and deployment guides.
    • Added detailed Docker development setup documentation.
    • Clarified deployment instructions and recommended setup methods.
  • Improvements

    • Implemented environment-specific Composer dependency installation.
    • Added CORS support and advanced proxy settings.
    • Refined development workflow with hot-reload capabilities.
    • Enhanced management of sensitive configuration files in version control.

kng and others added 4 commits January 27, 2025 22:30
Signed-off-by: Daniel Ekman <[email protected]>
Enhance Docker deployment documentation by:
- Adding detailed instructions for building Docker images
- Providing two methods for image building (Docker Compose and manual)
- Clarifying how to use local images with docker-compose.override.yml
Introduce development-specific Docker configuration:
- Create docker-compose.dev.yml for local development setup
- Add nginx.dev.conf with development CORS settings
- Update Dockerfile.api to support environment-specific dependency installation
- Configure development services with appropriate volumes and environment variables
- Remove platform-specific ARM64 constraints in docker-compose.dev.yml
- Enhance Nginx configuration with improved proxy and HMR settings
- Update documentation for development setup and Docker deployment
- Add new Docker development documentation page
- Refactor getting started guide with clearer development instructions
Copy link
Contributor

coderabbitai bot commented Jan 28, 2025

Walkthrough

This pull request introduces a comprehensive Docker-based development environment for the OpnForm application. It establishes a multi-service Docker setup with docker-compose.dev.yml and docker-compose.yml, defining configurations for API, UI, ingress, and worker services. The changes include new Nginx configurations, updated Dockerfiles, and expanded documentation to support Docker-based local development. The modifications aim to provide a more streamlined and consistent development experience across different platforms.

Changes

File Change Summary
docker-compose.dev.yml New multi-service development environment configuration with api, ui, ingress, and api-worker services
docker-compose.yml Added build configurations for api, api-worker, and db services with production-specific settings and health checks
docker/Dockerfile.api Introduced conditional Composer dependency installation based on APP_ENV with a multi-stage build process
docker/nginx.conf Updated document root and request handling in the Nginx configuration
docker/nginx.dev.conf New Nginx configuration for API requests and CORS handling
docs/contributing/getting-started.mdx Updated title and added development setup sections
docs/deployment/docker-development.mdx New documentation for Docker-based development setup
docs/deployment/docker.mdx Updated deployment guide with clearer build instructions
docs/deployment/local-deployment.mdx Added Docker development setup recommendation
docs/mint.json Added new documentation page for Docker development
.github/workflows/dockerhub.yml Enhanced workflow for publishing Docker images with new triggers and improved structure
.gitignore Added entries to ignore environment files while tracking example files

Possibly related PRs

  • 518 setup env command #519: The changes in this PR involve modifications to the docker-compose.yml file, which is directly related to the new docker-compose.dev.yml file introduced in the main PR, as both files define service configurations for the Docker setup.
  • Update Docker Compose commands in page documenting Docker Compose deployment #644: This PR updates documentation related to Docker Compose commands, which is relevant to the changes made in the main PR that establishes a new Docker development environment, ensuring users have the correct command syntax for the new setup.

🐰 In a world of code and Docker dreams,
We hop along with joyful themes.
New services bloom, configurations align,
A dev environment, oh so fine!
With Nginx and workers, all set to play,
Let’s code and create, come what may! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8dd60ae and 6d2a2ff.

📒 Files selected for processing (1)
  • .github/workflows/dockerhub.yml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/dockerhub.yml
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build the Nuxt app

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (12)
docker/nginx.conf (1)

17-30: Consider adding security headers for the proxy configuration.

While the proxy configuration is functional, consider enhancing security by adding the following headers:

 location / {
+    # Security headers
+    proxy_hide_header X-Powered-By;
+    proxy_set_header X-Content-Type-Options nosniff;
+    proxy_set_header X-XSS-Protection "1; mode=block";
     # Existing configuration...
 }
docker/nginx.dev.conf (2)

14-19: Consolidate CORS headers using a reusable configuration.

The CORS headers are duplicated between the main configuration and the preflight handler.

Consider using a reusable configuration:

+    # Define CORS configuration
+    map $http_origin $cors_origin {
+        default "";
+        "http://localhost:3000" "$http_origin";
+    }
+
     # Development CORS headers
-    add_header 'Access-Control-Allow-Origin' 'http://localhost:3000' always;
+    add_header 'Access-Control-Allow-Origin' $cors_origin always;

34-49: Consider rate limiting for API endpoints in development.

While this is a development configuration, it's good practice to maintain similar security measures as production.

     # Handle API requests
     location /api/ {
+        # Basic rate limiting
+        limit_req_zone $binary_remote_addr zone=api_limit:10m rate=5r/s;
+        limit_req zone=api_limit burst=10 nodelay;
+
         rewrite ^/api/(.*) /$1 break;
docs/contributing/getting-started.mdx (1)

10-14: Enhance Docker setup benefits description.

Consider adding more specific benefits to help developers make an informed choice.

 The easiest way to get started with OpnForm development is to use our Docker-based development environment. It provides:
 - Hot-reload for both frontend and backend
 - All necessary services pre-configured
 - Consistent development environment across all platforms
+- Built-in debugging capabilities
+- Easy service scaling and management
+- Isolated development environment
docker/Dockerfile.api (1)

Line range hint 1-1: Consider using PHP-FPM Alpine base image.

Using the Alpine-based image can significantly reduce the image size.

-FROM php:8.3-fpm
+FROM php:8.3-fpm-alpine
docker-compose.dev.yml (1)

52-62: Consider adding health checks for dependencies.

The ingress service depends on api and ui, but doesn't include health checks. This might cause issues if the dependent services aren't ready when ingress starts.

 depends_on:
-  - api
-  - ui
+  api:
+    condition: service_healthy
+  ui:
+    condition: service_healthy
docs/deployment/docker.mdx (1)

Line range hint 74-104: Consider adding security recommendations for custom builds.

While the build instructions are clear, consider adding security recommendations for users building their own images, such as:

  • Using multi-stage builds to reduce image size
  • Implementing least privilege principles
  • Scanning custom images for vulnerabilities
docs/deployment/docker-development.mdx (5)

23-25: Enhance prerequisites with version requirements.

Consider adding:

  • Minimum Docker version requirement
  • Minimum Docker Compose version requirement
  • Required disk space for the development environment

43-43: Consider simplifying repeated Docker Compose commands.

The document repeatedly uses the full docker compose -f docker-compose.yml -f docker-compose.dev.yml command. Consider adding a section about creating a Docker Compose alias or using the COMPOSE_FILE environment variable to simplify these commands.

Add this section before the Quick Start:

### Optional: Simplify Docker Compose Commands

Set the `COMPOSE_FILE` environment variable to avoid typing the full command:
```bash
export COMPOSE_FILE=docker-compose.yml:docker-compose.dev.yml

Then you can simply use docker compose up -d instead of the full command.



Also applies to: 94-94, 97-97, 106-106, 109-109, 128-128, 135-135, 138-138

---

`54-55`: **Consider security implications of default credentials.**

While convenient for development, having default credentials in the documentation might pose a security risk if users don't change them in production. Consider adding a warning note about this.

Add a security warning:

```markdown
<Warning>
These are development-only credentials. Ensure you change them immediately in production environments.
</Warning>

67-67: Clarify worker restart requirement.

The note about restarting the api-worker for queue jobs could be clearer. Consider explaining why this is necessary and in which specific scenarios.

Expand the explanation:

-Backend (Laravel): Changes to files in the `api` directory are immediately reflected, except for queued jobs which require restarting the api-worker container (`docker compose -f docker-compose.yml -f docker-compose.dev.yml restart api-worker`)
+Backend (Laravel): Changes to files in the `api` directory are immediately reflected. However, if you modify code related to queued jobs or the queue configuration, you'll need to restart the api-worker container:
+```bash
+docker compose restart api-worker
+```
+This is necessary because the worker process loads the job classes only once at startup.

121-139: Expand troubleshooting section.

Consider adding more common issues and their solutions:

  • Network conflicts (port 80/443/3000/5432 already in use)
  • Memory/CPU resource constraints
  • Common build errors
  • How to reset the development environment completely
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ae74f33 and 007886e.

📒 Files selected for processing (10)
  • docker-compose.dev.yml (1 hunks)
  • docker-compose.yml (3 hunks)
  • docker/Dockerfile.api (1 hunks)
  • docker/nginx.conf (1 hunks)
  • docker/nginx.dev.conf (1 hunks)
  • docs/contributing/getting-started.mdx (1 hunks)
  • docs/deployment/docker-development.mdx (1 hunks)
  • docs/deployment/docker.mdx (3 hunks)
  • docs/deployment/local-deployment.mdx (1 hunks)
  • docs/mint.json (1 hunks)
🔇 Additional comments (13)
docker/nginx.conf (2)

42-45: Verify path traversal protection for file access.

The location block for API and assets should include additional safeguards against path traversal attacks.

✅ Verification successful

Path traversal protection is adequately implemented

The current nginx configuration is secure against path traversal attacks by default because:

  • Uses proper root directive at server level
  • Employs try_files directive which safely resolves paths relative to root
  • Benefits from nginx's built-in path normalization and traversal prevention
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential path traversal vulnerabilities in the codebase
rg -g '*.php' '\.\./' ./

Length of output: 434


Script:

#!/bin/bash
# Check nginx configuration for security settings and path handling
echo "=== Checking nginx configuration ==="
rg -A 5 -B 5 "^\s*(root|alias|internal|deny|allow)" docker/
echo -e "\n=== Checking for security headers and path normalization ==="
rg "add_header|merge_slashes|resolve_symlinks|disable_symlinks" docker/

Length of output: 2520


47-54: Verify FastCGI script path security.

The hardcoded FastCGI script path might cause issues if the container structure changes.

Consider using $document_root instead of hardcoding the path:

-        fastcgi_param SCRIPT_FILENAME /usr/share/nginx/html/public/index.php;
+        fastcgi_param SCRIPT_FILENAME $document_root/index.php;
✅ Verification successful

FastCGI script path is secure but could be more maintainable

The current hardcoded path /usr/share/nginx/html/public/index.php is secure and correctly matches the Docker container structure. However, using $document_root would be more maintainable and consistent with the development configuration:

-        fastcgi_param SCRIPT_FILENAME /usr/share/nginx/html/public/index.php;
+        fastcgi_param SCRIPT_FILENAME $document_root/index.php;
🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found


Script:

#!/bin/bash
# Search for nginx configuration files
echo "=== Nginx Configuration Files ==="
fd -e conf -e conf.template

# Look for Docker-related files
echo -e "\n=== Docker-related Files ==="
fd -e dockerfile -e yml -e yaml | grep -i "docker"

# Display the complete nginx.conf content
echo -e "\n=== Content of nginx.conf ==="
cat docker/nginx.conf

# Look for PHP-FPM configurations
echo -e "\n=== PHP-FPM Related Files ==="
fd -g "*php-fpm*" -g "*www.conf*"

Length of output: 2234


Script:

#!/bin/bash
# Display Docker compose configurations
echo "=== Docker Compose Configuration ==="
cat docker-compose.yml
echo -e "\n=== Docker Compose Dev Configuration ==="
cat docker-compose.dev.yml

# Check nginx dev configuration
echo -e "\n=== Nginx Dev Configuration ==="
cat docker/nginx.dev.conf

Length of output: 6631

docker-compose.yml (2)

5-9: LGTM! Build configurations are well structured.

The build configurations are properly defined with correct context and environment arguments. Good use of separate Dockerfiles for API and client services.

Also applies to: 26-30, 42-44


Line range hint 11-19: Well organized environment configuration using YAML anchor.

Good practice using YAML anchor (&api-environment) to avoid duplication of environment variables between api and api-worker services.

docker-compose.dev.yml (3)

2-26: Good development setup for API service.

The configuration includes:

  • Correct volume mounts with vendor directory exclusion
  • Debug configuration with Xdebug support
  • Local environment settings

64-67: LGTM! Good use of YAML inheritance.

Efficiently reuses the api-base configuration for the worker service.


27-50: Verify HMR port availability.

The HMR port (24678) is exposed for Vite. Ensure this port doesn't conflict with other services in your development environment.

✅ Verification successful

Port 24678 is correctly configured for Vite HMR

The port configuration is consistent and doesn't conflict with any other services in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if port 24678 is commonly used by other services
rg -l "24678" --type yaml --type json

Length of output: 60

docs/deployment/local-deployment.mdx (1)

10-14: Clear and well-structured documentation update.

Good job highlighting Docker as the recommended approach while maintaining manual setup instructions as an alternative.

docs/mint.json (1)

76-76: LGTM! Documentation structure updated correctly.

The new docker-development page is properly integrated into the navigation structure.

docs/deployment/docker.mdx (1)

2-12: Clear distinction between production and development setups.

Good job clarifying the purpose of this guide and directing developers to the appropriate setup guide.

docs/deployment/docker-development.mdx (3)

37-38: Verify the setup script functionality.

Please ensure that the setup-env.sh script exists and properly creates the environment files.

✅ Verification successful

✅ The setup script is properly configured and functional

The setup-env.sh script exists, is executable, and properly handles environment setup by:

  • Creating environment files from appropriate templates
  • Generating secure keys and secrets
  • Supporting Docker-specific configurations with the --docker flag
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the setup script exists and is executable
fd "setup-env.sh" ./scripts

Length of output: 50


Script:

#!/bin/bash
# Check file permissions and content
ls -l ./scripts/setup-env.sh
echo "=== Script contents ==="
cat ./scripts/setup-env.sh

Length of output: 2881


1-139: Verify referenced files and configurations.

Please ensure all referenced files and configurations exist in the repository:

✅ Verification successful

Documentation accurately reflects repository structure

All referenced files, paths, and configurations in the Docker development documentation have been verified to exist in the repository:

  • scripts/setup-env.sh
  • Docker Compose files (docker-compose.yml, docker-compose.dev.yml)
  • Nginx configuration (docker/nginx.conf)
  • Environment templates (.env.example)
  • Environment variables documentation (docs/configuration/environment-variables.mdx)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for all referenced files and configurations
echo "Checking for required files..."

# Check Docker Compose files
fd "docker-compose\.(yml|dev\.yml)$"

# Check Nginx configuration
fd -t f "nginx\.conf$" --full-path

# Check environment templates
fd "\.env\.example$"

# List all volumes defined in Docker Compose files
echo "\nChecking volume definitions..."
grep -r "volumes:" .

Length of output: 625


Script:

#!/bin/bash
# Check setup script and service definitions
echo "Checking setup script..."
fd "setup-env\.sh$"

echo "\nChecking service definitions in compose files..."
echo "=== docker-compose.yml services ==="
grep -A 1 "services:" docker-compose.yml
echo "\n=== docker-compose.dev.yml services ==="
grep -A 1 "services:" docker-compose.dev.yml

echo "\nChecking for environment variables documentation..."
fd -t f "environment-variables\.(mdx|md)$" docs/

Length of output: 734


6-8: Verify the CloudVersion component implementation.

Please ensure that the imported CloudVersion component exists and properly handles the distinction between self-hosted and cloud versions of the documentation.

✅ Verification successful

CloudVersion component is correctly implemented and used

The component exists at ./docs/snippets/cloud-version.mdx and is consistently used across multiple documentation files to promote the cloud version of OpnForm. The implementation and usage are correct.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the CloudVersion component exists and its implementation
fd -e mdx -e tsx -e jsx "CloudVersion" ./docs/snippets

Length of output: 54


Script:

#!/bin/bash
# Search for the exact file and similar files
echo "=== Searching for cloud-version.mdx ==="
fd "cloud-version.mdx" .

echo -e "\n=== Searching for CloudVersion references ==="
rg -i "cloudversion" .

echo -e "\n=== Listing MDX files in snippets directories ==="
fd -e mdx . -d 2 | grep -i "snippets"

Length of output: 1447


Script:

#!/bin/bash
# Check the content of the CloudVersion component
cat ./docs/snippets/cloud-version.mdx

Length of output: 379

Comment on lines 33 to 39
# Install dependencies based on environment
ARG APP_ENV=production
RUN if [ "$APP_ENV" = "production" ]; then \
composer install --ignore-platform-req=php --no-dev --optimize-autoloader; \
else \
composer install --ignore-platform-req=php --optimize-autoloader; \
fi \
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize Dockerfile using multi-stage builds.

The current setup can be optimized to reduce the final image size and improve security.

Consider implementing a multi-stage build:

+FROM composer:latest as composer
+WORKDIR /app
+COPY api/composer.* ./
+RUN composer install --ignore-platform-req=php --no-dev --optimize-autoloader
+
+FROM php:8.3-fpm
 # Rest of the Dockerfile...
-COPY --from=composer:latest /usr/bin/composer /usr/local/bin/composer
-ENV COMPOSER_ALLOW_SUPERUSER=1
-# Install dependencies based on environment
-ARG APP_ENV=production
-RUN if [ "$APP_ENV" = "production" ]; then \
-    composer install --ignore-platform-req=php --no-dev --optimize-autoloader; \
-    else \
-    composer install --ignore-platform-req=php --optimize-autoloader; \
-    fi
+COPY --from=composer /app/vendor /usr/share/nginx/html/vendor

This approach:

  • Reduces final image size
  • Improves build caching
  • Separates build dependencies from runtime

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +115 to +117
- From containers: `host=db`
- From your machine: `localhost:5432`
- Default credentials: username=forge, password=forge, database=forge
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance database connection security.

The database credentials are hardcoded in the documentation. Consider:

  1. Using environment variables instead of hardcoded values
  2. Adding a note about changing these default values

- Update Docker Compose files with improved service configurations
- Add database healthcheck in docker-compose.yml
- Refactor GitHub Actions workflow for Docker image publishing
- Optimize Dockerfile.api with multi-stage build and environment-specific configurations
- Update Nginx configuration for development and production environments
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (7)
.github/workflows/dockerhub.yml (1)

21-31: Consider adding SHA-based tags for better traceability.

While the current version handling is good, consider adding commit SHA to tags for better debugging and rollback capabilities.

 if [[ $GITHUB_REF == refs/tags/* ]]; then
   echo "VERSION=${GITHUB_REF#refs/*/v}" >> $GITHUB_ENV
-  echo "API_TAGS=${{secrets.DOCKER_API_REPO}}:latest,${{secrets.DOCKER_API_REPO}}:${GITHUB_REF#refs/*/v}" >> $GITHUB_ENV
+  echo "API_TAGS=${{secrets.DOCKER_API_REPO}}:latest,${{secrets.DOCKER_API_REPO}}:${GITHUB_REF#refs/*/v},${{secrets.DOCKER_API_REPO}}:${GITHUB_SHA::7}" >> $GITHUB_ENV
 else
   echo "VERSION=dev" >> $GITHUB_ENV
-  echo "API_TAGS=${{secrets.DOCKER_API_REPO}}:dev" >> $GITHUB_ENV
+  echo "API_TAGS=${{secrets.DOCKER_API_REPO}}:dev,${{secrets.DOCKER_API_REPO}}:${GITHUB_SHA::7}" >> $GITHUB_ENV
 fi
docker-compose.dev.yml (1)

36-39: Consider using a more resilient command for UI service.

The current command might fail if package.json changes. Consider using a more robust approach.

-    command: sh -c "npm install && NITRO_HOST=0.0.0.0 NITRO_PORT=3000 npm run dev"
+    command: sh -c "if [ ! -d node_modules ] || [ ! -f node_modules/.package-lock.json ] || [ package-lock.json -nt node_modules/.package-lock.json ]; then
+      npm ci && cp package-lock.json node_modules/.package-lock.json;
+    fi && NITRO_HOST=0.0.0.0 NITRO_PORT=3000 npm run dev"
docker/nginx.dev.conf (5)

1-4: Enhance URI mapping security with stricter regex pattern.

The current regex pattern ~^/api(/.*$) accepts any character after /api/. Consider using a more restrictive pattern to explicitly allow only expected characters.

 map $original_uri $api_uri {
-    ~^/api(/.*$) $1;
+    ~^/api(/[a-zA-Z0-9\-_/]+$) $1;
     default $original_uri;
 }

6-14: Add security headers to enhance protection.

While the basic configuration is good, consider adding security headers to protect against common web vulnerabilities.

     error_log  /dev/stderr error;
 
+    # Security headers
+    add_header X-Content-Type-Options "nosniff" always;
+    add_header X-Frame-Options "SAMEORIGIN" always;
+    add_header X-XSS-Protection "1; mode=block" always;
+    add_header Referrer-Policy "strict-origin-when-cross-origin" always;
+
     index index.html index.htm index.php;

51-59: Align WebSocket configuration with main proxy settings.

The WebSocket configuration should maintain consistency with the main proxy configuration by including the same timeout and buffering settings.

         proxy_set_header Connection "upgrade";
         proxy_set_header Host $host;
         proxy_cache_bypass $http_upgrade;
+        proxy_read_timeout 300;
+        proxy_buffering off;

61-63: Enhance asset handling with caching and security measures.

The current configuration treats all paths similarly. Consider adding specific handling for static assets with appropriate caching headers and file type restrictions.

     location ~/(api|open|local\/temp|forms\/assets)/ {
+        # Add caching for static assets
+        location ~* \.(jpg|jpeg|png|gif|ico|css|js)$ {
+            expires 7d;
+            add_header Cache-Control "public, no-transform";
+        }
+
         try_files $uri $uri/ /index.php?$query_string;
     }

65-72: Enhance PHP-FPM configuration with security and performance optimizations.

Add security measures and performance optimizations to the PHP processing configuration.

     location ~ \.php$ {
         fastcgi_split_path_info ^(.+\.php)(/.+)$;
         fastcgi_pass api:9000;
         fastcgi_index index.php;
         include fastcgi_params;
+        # Security: Deny access to hidden files
+        location ~ /\. {
+            deny all;
+        }
+        # Performance optimizations
+        fastcgi_keep_conn on;
+        fastcgi_busy_buffers_size 256k;
+        fastcgi_buffer_size 128k;
+        fastcgi_buffers 4 256k;
         fastcgi_param SCRIPT_FILENAME /usr/share/nginx/html/public/index.php;
         fastcgi_param REQUEST_URI $api_uri;
     }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 007886e and 8dd60ae.

📒 Files selected for processing (7)
  • .github/workflows/dockerhub.yml (1 hunks)
  • .gitignore (1 hunks)
  • docker-compose.dev.yml (1 hunks)
  • docker-compose.yml (3 hunks)
  • docker/Dockerfile.api (1 hunks)
  • docker/nginx.conf (1 hunks)
  • docker/nginx.dev.conf (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docker/nginx.conf
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Build the Nuxt app
🔇 Additional comments (11)
.gitignore (1)

14-22: LGTM! Environment file management is properly configured.

The .gitignore entries correctly handle environment files by:

  • Ignoring all .env files and variations
  • Preserving .env.example files for reference
  • Covering all relevant directories (root, api/, client/)
docker-compose.yml (2)

8-9: LGTM! Build arguments are properly configured.

The APP_ENV build argument is correctly set to production for both api and api-worker services, ensuring consistent environment configuration.

Also applies to: 29-30


57-61: LGTM! Database healthcheck is properly configured.

The healthcheck configuration:

  • Uses the recommended pg_isready command
  • Has appropriate timing parameters
  • Correctly uses environment variables for credentials
docker/Dockerfile.api (3)

1-11: LGTM! Multi-stage build is properly implemented.

The first stage correctly handles Composer dependencies with:

  • Separate build stage for dependencies
  • Environment-aware installation
  • Proper working directory setup

30-35: LGTM! Development tools are properly configured.

Xdebug installation is:

  • Only enabled in non-production environments
  • Properly integrated with PHP extensions

58-61: LGTM! File permissions are properly set.

The permissions are correctly set for:

  • Storage directory (775)
  • Cache directory (775)
  • Ownership (www-data)
.github/workflows/dockerhub.yml (2)

7-13: LGTM! Workflow triggers are well configured.

The triggers are properly set up for:

  • Tag releases
  • Main branch changes
  • Relevant path filters

48-57: LGTM! Build configuration is properly set up.

The build configuration includes:

  • Multi-platform support
  • Proper build arguments
  • Correct environment handling
docker-compose.dev.yml (2)

9-12: LGTM! Volume mounts are properly configured.

The volume mounts are correctly set up:

  • API code mounted with delegated mode
  • Vendor directory properly excluded
  • Storage directory separately mounted

71-77: LGTM! Worker service is properly configured.

The api-worker service:

  • Inherits base configuration correctly
  • Has proper environment variable set
  • Depends on database health
docker/nginx.dev.conf (1)

35-49: Review the proxy read timeout value.

The current proxy_read_timeout of 86400 seconds (24 hours) is extremely high. While this might be intended for development, consider using a more reasonable timeout value to prevent potential resource exhaustion.

Consider reducing it to a more reasonable value like 300 seconds (5 minutes):

-        proxy_read_timeout 86400;
+        proxy_read_timeout 300;

Comment on lines +16 to +33
# Development CORS headers
add_header 'Access-Control-Allow-Origin' 'http://localhost:3000' always;
add_header 'Access-Control-Allow-Methods' 'GET, POST, PUT, DELETE, OPTIONS, PATCH' always;
add_header 'Access-Control-Allow-Headers' 'DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range,Authorization,X-XSRF-TOKEN' always;
add_header 'Access-Control-Allow-Credentials' 'true' always;
add_header 'Access-Control-Expose-Headers' 'Content-Length,Content-Range' always;

# Handle preflight requests
if ($request_method = 'OPTIONS') {
add_header 'Access-Control-Allow-Origin' 'http://localhost:3000' always;
add_header 'Access-Control-Allow-Methods' 'GET, POST, PUT, DELETE, OPTIONS, PATCH' always;
add_header 'Access-Control-Allow-Headers' 'DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range,Authorization,X-XSRF-TOKEN' always;
add_header 'Access-Control-Allow-Credentials' 'true' always;
add_header 'Access-Control-Max-Age' 1728000;
add_header 'Content-Type' 'text/plain charset=UTF-8';
add_header 'Content-Length' 0;
return 204;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor CORS configuration to reduce duplication and improve flexibility.

The CORS headers are duplicated between the main configuration and OPTIONS block. Consider using variables to maintain consistency and make the configuration more environment-aware.

+    # CORS configuration
+    set $cors_origin 'http://localhost:3000';
+    set $cors_methods 'GET, POST, PUT, DELETE, OPTIONS, PATCH';
+    set $cors_headers 'DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range,Authorization,X-XSRF-TOKEN';
+
     # Development CORS headers
-    add_header 'Access-Control-Allow-Origin' 'http://localhost:3000' always;
-    add_header 'Access-Control-Allow-Methods' 'GET, POST, PUT, DELETE, OPTIONS, PATCH' always;
-    add_header 'Access-Control-Allow-Headers' 'DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range,Authorization,X-XSRF-TOKEN' always;
+    add_header 'Access-Control-Allow-Origin' $cors_origin always;
+    add_header 'Access-Control-Allow-Methods' $cors_methods always;
+    add_header 'Access-Control-Allow-Headers' $cors_headers always;
     add_header 'Access-Control-Allow-Credentials' 'true' always;
     add_header 'Access-Control-Expose-Headers' 'Content-Length,Content-Range' always;

     # Handle preflight requests
     if ($request_method = 'OPTIONS') {
-        add_header 'Access-Control-Allow-Origin' 'http://localhost:3000' always;
-        add_header 'Access-Control-Allow-Methods' 'GET, POST, PUT, DELETE, OPTIONS, PATCH' always;
-        add_header 'Access-Control-Allow-Headers' 'DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range,Authorization,X-XSRF-TOKEN' always;
+        add_header 'Access-Control-Allow-Origin' $cors_origin always;
+        add_header 'Access-Control-Allow-Methods' $cors_methods always;
+        add_header 'Access-Control-Allow-Headers' $cors_headers always;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Development CORS headers
add_header 'Access-Control-Allow-Origin' 'http://localhost:3000' always;
add_header 'Access-Control-Allow-Methods' 'GET, POST, PUT, DELETE, OPTIONS, PATCH' always;
add_header 'Access-Control-Allow-Headers' 'DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range,Authorization,X-XSRF-TOKEN' always;
add_header 'Access-Control-Allow-Credentials' 'true' always;
add_header 'Access-Control-Expose-Headers' 'Content-Length,Content-Range' always;
# Handle preflight requests
if ($request_method = 'OPTIONS') {
add_header 'Access-Control-Allow-Origin' 'http://localhost:3000' always;
add_header 'Access-Control-Allow-Methods' 'GET, POST, PUT, DELETE, OPTIONS, PATCH' always;
add_header 'Access-Control-Allow-Headers' 'DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range,Authorization,X-XSRF-TOKEN' always;
add_header 'Access-Control-Allow-Credentials' 'true' always;
add_header 'Access-Control-Max-Age' 1728000;
add_header 'Content-Type' 'text/plain charset=UTF-8';
add_header 'Content-Length' 0;
return 204;
}
# CORS configuration
set $cors_origin 'http://localhost:3000';
set $cors_methods 'GET, POST, PUT, DELETE, OPTIONS, PATCH';
set $cors_headers 'DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range,Authorization,X-XSRF-TOKEN';
# Development CORS headers
add_header 'Access-Control-Allow-Origin' $cors_origin always;
add_header 'Access-Control-Allow-Methods' $cors_methods always;
add_header 'Access-Control-Allow-Headers' $cors_headers always;
add_header 'Access-Control-Allow-Credentials' 'true' always;
add_header 'Access-Control-Expose-Headers' 'Content-Length,Content-Range' always;
# Handle preflight requests
if ($request_method = 'OPTIONS') {
add_header 'Access-Control-Allow-Origin' $cors_origin always;
add_header 'Access-Control-Allow-Methods' $cors_methods always;
add_header 'Access-Control-Allow-Headers' $cors_headers always;
add_header 'Access-Control-Allow-Credentials' 'true' always;
add_header 'Access-Control-Max-Age' 1728000;
add_header 'Content-Type' 'text/plain charset=UTF-8';
add_header 'Content-Length' 0;
return 204;
}

Configure GitHub Actions workflow with explicit read and write permissions for content and packages to improve security and clarity of Docker image deployment process
@JhumanJ JhumanJ merged commit bf85d8f into main Jan 28, 2025
7 checks passed
@JhumanJ JhumanJ deleted the developer-docker-setup branch January 28, 2025 16:52
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.

2 participants