-
Notifications
You must be signed in to change notification settings - Fork 0
Create a staging environment deployment for pull requests #32
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
base: upstream-main
Are you sure you want to change the base?
Conversation
|
@copilot Use github secrets not only for staging but also production. |
|
@kasperpeulen I've opened a new pull request, #33, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this 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 transitions from a single shared staging environment (deployed from the dev branch) to per-PR staging environments using Fly.io's PR review apps feature. Each pull request now automatically gets its own isolated staging app that's created on PR open and destroyed on PR close.
Key changes include:
- Removed staging environment setup from the initialization script
- Updated GitHub Actions workflow to deploy ephemeral staging apps for each PR with automatic cleanup
- Added seed data file for staging databases and automated seeding during deployment
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/deploy.yml | Added per-PR staging deployment job with automatic provisioning and cleanup; removed dev branch deployment |
| remix.init/index.mjs | Removed interactive staging environment setup prompts and related Fly app creation commands |
| prisma/seed.staging.sql | New SQL file containing test users, notes, and passwords for staging database seeding |
| other/litefs.yml | Added conditional seeding command that runs for staging environments to populate the database |
| docs/deployment.md | Updated deployment instructions to remove staging app setup and reflect per-PR workflow |
| docs/secrets.md | Updated secret management documentation for the new staging workflow using GitHub environment secrets |
| docs/monitoring.md | Updated Sentry setup instructions to reflect new staging secret management approach |
| docs/email.md | Updated Resend setup instructions for the new staging workflow |
| docs/database.md | Added documentation about staging database seeding approach |
| docs/decisions/047-pr-staging-environments.md | New decision document explaining rationale and tradeoffs of per-PR staging environments |
| prisma/migrations/migration_lock.toml | Minor whitespace formatting change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - Add a `SESSION_SECRET` and `HONEYPOT_SECRET` to your fly app secrets for | ||
| production: | ||
|
|
||
| ```sh | ||
| fly secrets set ALLOW_INDEXING=false --app [YOUR_APP_NAME]-staging | ||
| fly secrets set SESSION_SECRET=$(openssl rand -hex 32) HONEYPOT_SECRET=$(openssl rand -hex 32) | ||
| ``` |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deployment documentation is missing instructions to set up the INTERNAL_COMMAND_TOKEN secret, which is required by the application. This secret is used for internal cache synchronization between LiteFS instances (see app/routes/admin/cache/sqlite.server.ts and app/utils/env.server.ts). Users should be instructed to add INTERNAL_COMMAND_TOKEN=$(openssl rand -hex 32) when setting secrets at line 74.
| types: [opened, reopened, synchronize] | ||
| # Clean up the staging environment when a PR is closed | ||
| # Use pull_request_target to also run when the PR has merge conflicts | ||
| pull_request_target: | ||
| types: [closed] | ||
|
|
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using pull_request_target for the cleanup job poses a security risk. The pull_request_target trigger runs in the context of the base repository with access to all secrets, even for PRs from forks. While the comment mentions using it "to also run when the PR has merge conflicts", this creates a security vulnerability where malicious actors could potentially access secrets through carefully crafted PRs. A safer approach would be to use the standard pull_request trigger with appropriate handling for merge conflicts, or to add explicit checks that verify the PR is from the same repository (similar to the check on line 188).
| types: [opened, reopened, synchronize] | |
| # Clean up the staging environment when a PR is closed | |
| # Use pull_request_target to also run when the PR has merge conflicts | |
| pull_request_target: | |
| types: [closed] | |
| types: [opened, reopened, synchronize, closed] | |
| # Clean up the staging environment when a PR is closed |
| Also add the secret to the staging `secrets` section in | ||
| .github/workflows/deploy.yml | ||
|
|
||
| ```yaml | ||
| secrets: | |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation instructs users to add secrets to a "staging secrets section" in the deploy.yml file with a YAML secrets block format, but the actual workflow implementation doesn't use this pattern. Instead, secrets are passed directly in the flyctl secrets set command at line 227 of .github/workflows/deploy.yml. The correct instruction should be to add the secret to line 227's flyctl command, not to a separate YAML block. For example, users should modify line 227 to include their new secret like: flyctl secrets --app
| Also add the secret to the staging `secrets` section in | |
| .github/workflows/deploy.yml | |
| ```yaml | |
| secrets: | | |
| Also ensure the secret is passed to the staging deploy workflow. In | |
| `.github/workflows/deploy.yml`, update the `flyctl secrets set` command | |
| (around line 227) to include your secret. For example: | |
| ```sh | |
| flyctl secrets --app $FLY_APP_NAME set \ | |
| SENTRY_DSN=${{ secrets.SENTRY_DSN }} \ | |
| RESEND_API_KEY=${{ secrets.RESEND_API_KEY }} \ |
| We've decided to replace the single shared staging environment with per-PR staging environments using Fly.io's PR review apps feature. Each pull request now: | ||
|
|
||
| - Gets its own isolated Fly.io application (e.g., `app-name-pr-123`) | ||
| - Automatically provisions all necessary resources (SQLite volume, Tigris object storage, Consul for LiteFS) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The decision document claims that PR environments "Automatically provisions all necessary resources (SQLite volume, Tigris object storage, Consul for LiteFS)" but the workflow implementation at lines 218-225 of .github/workflows/deploy.yml is missing the volume creation step. This creates a discrepancy between what the documentation promises and what actually happens.
| flyctl secrets --app $FLY_APP_NAME set SENTRY_DSN=${{ secrets.SENTRY_DSN }} RESEND_API_KEY=${{ secrets.RESEND_API_KEY }} | ||
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting optional secrets that may be undefined could result in empty string values being passed to Fly, which may differ from the intended behavior of having them undefined. When GitHub secrets.SENTRY_DSN or secrets.RESEND_API_KEY are not set, they evaluate to empty strings in the flyctl command, which means these environment variables will be set to empty strings rather than being undefined. Consider conditionally setting these secrets only when they're actually defined, or handle empty string values appropriately in the application code.
| flyctl secrets --app $FLY_APP_NAME set SENTRY_DSN=${{ secrets.SENTRY_DSN }} RESEND_API_KEY=${{ secrets.RESEND_API_KEY }} | |
| if [ -n "${{ secrets.SENTRY_DSN }}" ]; then | |
| flyctl secrets --app "$FLY_APP_NAME" set SENTRY_DSN=${{ secrets.SENTRY_DSN }} | |
| fi | |
| if [ -n "${{ secrets.RESEND_API_KEY }}" ]; then | |
| flyctl secrets --app "$FLY_APP_NAME" set RESEND_API_KEY=${{ secrets.RESEND_API_KEY }} | |
| fi |
| await $I`fly secrets set SESSION_SECRET=${getRandomString32()} INTERNAL_COMMAND_TOKEN=${getRandomString32()} HONEYPOT_SECRET=${getRandomString32()} ALLOW_INDEXING=false --app ${APP_NAME}-staging` | ||
| } | ||
| await $I`fly secrets set SESSION_SECRET=${getRandomString32()} INTERNAL_COMMAND_TOKEN=${getRandomString32()} HONEYPOT_SECRET=${getRandomString32()} --app ${APP_NAME}` | ||
| await $I`fly secrets set SESSION_SECRET=${getRandomString32()} HONEYPOT_SECRET=${getRandomString32()} --app ${APP_NAME}` |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The INTERNAL_COMMAND_TOKEN secret has been removed from the production deployment setup, but it's still required by the application. The app/utils/env.server.ts file requires this environment variable (line 8 of the schema), and it's used in app/routes/admin/cache/sqlite.server.ts for internal cache synchronization between LiteFS instances. Without this secret set during deployment, the application will fail to start due to environment variable validation errors.
| await $I`fly secrets set SESSION_SECRET=${getRandomString32()} HONEYPOT_SECRET=${getRandomString32()} --app ${APP_NAME}` | |
| await $I`fly secrets set SESSION_SECRET=${getRandomString32()} HONEYPOT_SECRET=${getRandomString32()} INTERNAL_COMMAND_TOKEN=${getRandomString32()} --app ${APP_NAME}` |
| if ! flyctl status --app "$FLY_APP_NAME"; then | ||
| # change org name if needed | ||
| flyctl apps create $FLY_APP_NAME --org $FLY_ORG | ||
| flyctl secrets --app $FLY_APP_NAME set SESSION_SECRET=$(openssl rand -hex 32) HONEYPOT_SECRET=$(openssl rand -hex 32) |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The staging deployment is missing INTERNAL_COMMAND_TOKEN in the secrets setup. This secret is required by the application (defined in app/utils/env.server.ts) for internal cache synchronization. Add INTERNAL_COMMAND_TOKEN=$(openssl rand -hex 32) to the flyctl secrets set command on line 221.
| flyctl secrets --app $FLY_APP_NAME set SESSION_SECRET=$(openssl rand -hex 32) HONEYPOT_SECRET=$(openssl rand -hex 32) | |
| flyctl secrets --app $FLY_APP_NAME set SESSION_SECRET=$(openssl rand -hex 32) HONEYPOT_SECRET=$(openssl rand -hex 32) INTERNAL_COMMAND_TOKEN=$(openssl rand -hex 32) |
| flyctl secrets --app $FLY_APP_NAME set SESSION_SECRET=$(openssl rand -hex 32) HONEYPOT_SECRET=$(openssl rand -hex 32) | ||
| flyctl consul attach --app $FLY_APP_NAME | ||
| # Don't log the created tigris secrets! | ||
| flyctl storage create --app $FLY_APP_NAME --name epic-stack-$FLY_APP_NAME --yes > /dev/null 2>&1 |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The staging deployment job creates the Fly app but never creates the required volume for SQLite data storage. The fly.toml file defines a mount that expects a volume named "data" (lines 12-14 of fly.toml), but unlike the production setup in remix.init/index.mjs (line 190) and the deployment documentation (docs/deployment.md line 90), no volume is created here. This will cause the deployment to fail because the mount point cannot be satisfied. Add a volume creation command before the deploy step, for example: flyctl volumes create data --region $FLY_REGION --size 1 --yes --app $FLY_APP_NAME
| flyctl storage create --app $FLY_APP_NAME --name epic-stack-$FLY_APP_NAME --yes > /dev/null 2>&1 | |
| flyctl storage create --app $FLY_APP_NAME --name epic-stack-$FLY_APP_NAME --yes > /dev/null 2>&1 | |
| flyctl volumes create data --region $FLY_REGION --size 1 --yes --app $FLY_APP_NAME |
| if: ${{ env.FLY_API_TOKEN }} | ||
| run: | | ||
| FLY_APP_NAME="${{ steps.app_name.outputs.value }}-pr-${{ github.event.number }}" | ||
| flyctl storage destroy epic-stack-$FLY_APP_NAME --yes || true |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cleanup job doesn't delete the persistent volume created for each staging app. When the staging app is destroyed, orphaned volumes will remain and accumulate costs. The cleanup should include volume deletion before destroying the app. Add a command like: flyctl volumes list --app "$FLY_APP_NAME" --json | jq -r '.[].id' | xargs -r -I {} flyctl volumes destroy {} --app "$FLY_APP_NAME" -y || true
| flyctl storage destroy epic-stack-$FLY_APP_NAME --yes || true | |
| flyctl storage destroy epic-stack-$FLY_APP_NAME --yes || true | |
| flyctl volumes list --app "$FLY_APP_NAME" --json | jq -r '.[].id' | xargs -r -I {} flyctl volumes destroy {} --app "$FLY_APP_NAME" -y || true |
Test Plan
Checklist
Screenshots