Skip to content

fix: update demo script#1862

Merged
benjamin-747 merged 1 commit into
gitmono-dev:mainfrom
benjamin-747:main
Jan 29, 2026
Merged

fix: update demo script#1862
benjamin-747 merged 1 commit into
gitmono-dev:mainfrom
benjamin-747:main

Conversation

@benjamin-747

Copy link
Copy Markdown
Collaborator

No description provided.

@benjamin-747 benjamin-747 requested a review from genedna January 29, 2026 02:08
@github-actions

github-actions Bot commented Jan 29, 2026

Copy link
Copy Markdown

Claude finished @benjamin-747's task in 2m 19s —— View job


Code Review Complete ✓

Tasks

  • Read repository guidelines (CLAUDE.md)
  • Analyze PR changes and file diffs
  • Review code quality and best practices
  • Check for security concerns
  • Evaluate performance implications
  • Assess testing coverage
  • Review documentation updates
  • Provide comprehensive feedback

Summary

This PR updates the demo environment configuration and deployment scripts. While the changes improve the demo setup with better environment variable naming and network configuration, there are 2 critical bugs that will prevent the deployment from working.

🚨 Critical Issues (Must Fix)

  1. Workflow will fail: Platform mismatch in manifest creation (.github/workflows/web-deploy.yml:110)

    • The build job only creates linux/amd64 images
    • The manifest job tries to reference both amd64 AND arm64 for demo
    • This will fail because arm64 images don't exist
    • Impact: CI/CD pipeline will break on the manifest step
  2. S3 credentials won't work: Inconsistent environment variable names (docker-compose.demo.yml:134-135)

    • Lines 134-135 use MEGA_OBJECT__S3__* (missing STORAGE)
    • Line 129 uses MEGA_OBJECT_STORAGE__S3__* (correct format)
    • Impact: Mega backend won't find S3 credentials, causing storage failures

⚠️ High Priority Issues

  1. Security risk: Removed repository check (.github/workflows/web-deploy.yml:28)
    • Removed if: ${{ github.repository == 'web3infra-foundation/mega' }}
    • Workflow will now run in forks, potentially exposing secrets
    • Impact: Security risk + failed deployments in forks

💡 Questions & Concerns

  1. Missing UI environment variables (docker-compose.demo.yml:189-193)

    • Removed MEGA_INTERNAL_HOST, MEGA_HOST, HOSTNAME, and PORT from mega_ui service
    • Are these now baked into the Docker image via APP_ENV=demo build arg?
    • Needs clarification: How does the UI know where to connect?
  2. Alpha version pinning (docker-compose.demo.yml:76)

    • RustFS pinned to 1.0.0-alpha.81
    • Why use an alpha version instead of stable?
    • Recommendation: Document version requirements

✅ Good Changes

  • Improved configuration structure: Environment variables now use hierarchical MEGA_OBJECT_STORAGE__S3__* format
  • Better network setup: Added network aliases (git.gitmono.local, app.gitmono.local, etc.) for easier local testing
  • CSP updates: Added *.gitmono.local domains to Next.js CSP for local demo connectivity
  • Redis configuration: Added REDIS_URL to Campsite API environment
  • Error handling: Added set -euo pipefail to workflow scripts
  • Build arg support: Added APP_ENV=demo build argument for mega-ui
  • MySQL compatibility: Added mysql_native_password plugin for broader client support
  • Debug logging: Added build args logging in local build script

📋 Recommendations

  1. Fix the critical bugs first before merging
  2. Add comments explaining APP_ENV behavior and RustFS version constraints
  3. Consider re-adding the repository check for security
  4. Add integration tests to verify demo environment startup
  5. Document the environment variable changes in a migration guide

🔍 Testing Coverage

Missing: No tests were added or modified. Consider adding:

  • Integration test for demo environment startup
  • Validation test for environment variable configuration
  • CI test to verify manifest creation succeeds

📖 Documentation

Missing: No documentation updates. Consider documenting:

  • Environment variable naming changes (migration guide)
  • APP_ENV build argument behavior
  • RustFS version requirements
  • Network alias usage for local testing

Action Items

To make this PR production-ready:

  1. Fix environment variable naming in docker-compose.demo.yml (lines 134-135)
  2. Fix workflow platform mismatch (remove arm64 from demo manifest OR add arm64 builds)
  3. Clarify UI configuration approach (image vs environment variables)
  4. Re-add repository security check or document why it's safe to remove
  5. Add documentation for breaking changes

@@ -26,7 +26,6 @@ concurrency:

jobs:
build-push-single-arch:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Security Concern: Removing the repository check if: ${{ github.repository == 'web3infra-foundation/mega' }} means this workflow will now run in forks. This could lead to:

  1. Failed deployments in forks (they won't have the AWS credentials)
  2. Potential secret exposure attempts
  3. Unnecessary CI/CD costs

Recommendation: Keep this guard clause to ensure the workflow only runs in the main repository.

OBJECT_STORAGE_S3__SECRET_ACCESS_KEY: ${S3_SECRET_ACCESS_KEY:-rustfsadmin}
MEGA_OBJECT_STORAGE__S3__BUCKET: ${MEGA_OBJECT_STORAGE__S3__BUCKET:-mega}
MEGA_OBJECT_STORAGE__S3__REGION: ${MEGA_OBJECT_STORAGE__S3__REGION:-us-east-1}
MEGA_OBJECT__S3__ACCESS_KEY_ID: ${S3_ACCESS_KEY_ID:-rustfsadmin}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical Bug: Environment variable names are inconsistent! Lines 134-135 use:

  • MEGA_OBJECT__S3__ACCESS_KEY_ID
  • MEGA_OBJECT__S3__SECRET_ACCESS_KEY

But line 129 uses MEGA_OBJECT_STORAGE__S3__ENDPOINT_URL. This inconsistency will break S3 connectivity because the Mega backend won't find the credentials.

Expected: All should use either MEGA_OBJECT_STORAGE__S3__* or MEGA_OBJECT__S3__* (the former seems correct based on line 129).

Comment thread docker/demo/.env.example
OBJECT_STORAGE_S3__ENDPOINT_URL=http://rustfs:9000
OBJECT_STORAGE_S3__BUCKET=mega
OBJECT_STORAGE_S3__REGION=us-east-1
MEGA_OBJECT_STORAGE__S3__ENDPOINT_URL=http://rustfs:9000

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good Change: The environment variable names have been properly updated to use the MEGA_OBJECT_STORAGE__S3__* prefix, which aligns with the hierarchical config structure. This is more maintainable than the old OBJECT_STORAGE_S3__* format.

Comment thread docker/demo/docker-compose.demo.yml
Comment thread docker/demo/docker-compose.demo.yml
# This guarantees the pushed artifact is a single-architecture image manifest.
build_args+=(--load)

if [ "$image_name" = "mega-ui" ]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good Addition: Adding APP_ENV=demo build arg for mega-ui ensures the Next.js app is built with demo-specific configuration. This aligns with the changes in the GitHub workflow and environment setup.

Suggestion: Consider documenting what APP_ENV=demo does - does it set different API endpoints? Does it use different build outputs?

'https://*.gitmono.com',
'wss://*.gitmono.com',
// Local demo environments
'http://*.gitmono.local:8004',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good Security Practice: Adding *.gitmono.local URLs to the CSP connect-src directive allows the Next.js app to communicate with local demo services. The specific ports (8004, 8000, 8080) correspond to:

  • 8004: Orion Server
  • 8000: Mega Backend
  • 8080: Campsite API

This is a safe addition for local development/demo environments.

env:
REGISTRY: ${{ steps.login-ecr-public.outputs.registry }}
run: |
set -euo pipefail

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Quality: Good addition of set -euo pipefail to fail fast on errors. This prevents silent failures in the manifest creation step.

However, the script still has a logic issue (see my other comment about platform mismatch).

Comment thread .github/workflows/web-deploy.yml
redis:
condition: service_healthy
environment:
REDIS_URL: ${CAMPSITE_REDIS_URL:-redis://redis:6379}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good Addition: Adding REDIS_URL environment variable to Campsite API ensures it can connect to Redis properly. This was missing before and aligns with the .env.example changes.

mysql:
image: mysql:8.0
container_name: mega-demo-mysql
command:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Security Note: Adding --default-authentication-plugin=mysql_native_password uses the older MySQL authentication method instead of the more secure caching_sha2_password (MySQL 8.0 default).

This is acceptable for demo purposes but should be documented. Some older MySQL clients don't support the new auth plugin, so this maintains compatibility.

# Add build context
build_args+=("${build_context}")

log_info "output build args: ${build_args[*]}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Quality: Good addition of debug logging to show the build args being used. This will help troubleshoot build issues.

Minor suggestion: Consider using a more descriptive log prefix like "Build arguments:" instead of "output build args:"

@benjamin-747 benjamin-747 added this pull request to the merge queue Jan 29, 2026
Merged via the queue into gitmono-dev:main with commit 8824850 Jan 29, 2026
8 checks passed
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