-
Notifications
You must be signed in to change notification settings - Fork 27
Explicitly configure flyio mtcl.cc domain #4134
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: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThe PR updates the CI/CD preview workflow to replace the default Fly.io domain with a Cloudflare-managed preview domain, adds certificate registration via flyctl, and updates the corresponding URL configuration used by the deployment pipeline. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/pr_preview.yml:
- Around line 292-299: The workflow currently swallows errors by using `flyctl
certs add "${APP_DOMAIN}" --app "${APP_NAME}" || true`; instead, query Fly for
existing certs using `flyctl certs list --json` and `jq` to check if APP_DOMAIN
already has a cert, skip adding if it exists, otherwise run `flyctl certs add
"${APP_DOMAIN}" --app "${APP_NAME}"` and allow its non-zero exit code to fail
the job; replace the `|| true` pattern with this JSON-based existence check and
proper exit propagation so auth/DNS/rate-limit errors are not ignored.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr_preview.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Docker Image
- GitHub Check: integration-tests
- GitHub Check: Backend Checks
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| # Define app URLs (used for secrets and outputs) | ||
| APP_DOMAIN="${APP_NAME}.fly.dev" | ||
| # Custom domain via Cloudflare Worker: {fly_app}-preview.mtcl.cc | ||
| APP_DOMAIN="${APP_NAME}-preview.mtcl.cc" | ||
| PUBLIC_APP_URL="https://${APP_DOMAIN}" | ||
| # Register custom domain with Fly.io (Cloudflare handles SSL) | ||
| flyctl certs add "${APP_DOMAIN}" --app "${APP_NAME}" || true | ||
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.
🧩 Analysis chain
🌐 Web query:
flyctl certs add exit code behavior when certificate already exists
💡 Result:
Short answer: flyctl certs add fails with an error (prints "Error: Hostname already exists on app") and returns a non‑zero exit status rather than silently succeeding — you must remove or re-use the existing certificate (or target the correct app) before adding the same hostname. [1][2]
Sources:
- Fly docs: certs add (usage/info). [1]
- Fly community threads showing the "Hostname already exists on app" error when re-adding an existing cert. [2]
🌐 Web query:
flyctl certs add documentation exit codes
💡 Result:
Short answer: Fly's official docs for "fly certs add" do not document any CLI exit codes. The command page shows usage and flags but no exit-status information. [1]
If you need exit-code behaviour for scripting, check the flyctl source on GitHub (superfly/flyctl) or run the specific flyctl version and test cases (e.g., missing args, auth failure) to observe returned statuses — the docs don't list them. [2]
Sources:
- flyctl certs add documentation. [1]
- superfly/flyctl GitHub repository (source for CLI behavior). [2]
🌐 Web query:
flyctl certs list output format grep
💡 Result:
By default fly certs list prints a human-readable table. For reliable scripting/grepping use JSON output (-j) and jq; example:
-
Human-readable (quick grep):
fly certs list -a myapp | grep example.com -
Reliable JSON parsing:
fly certs list -a myapp -j | jq -r '.[] | .hostname'
Notes:
- The CLI supports -j/--json for machine-readable output. [1]
- The API/GraphQL responses show certificates under app.certificates.nodes (useful when using the API). [2]
Sources: Fly docs — fly certs list; Fly docs — custom domain / certificates API. [1][2]
Don't swallow Fly cert registration failures.
flyctl certs add ... || true ignores real failures (auth/DNS/rate limits), potentially leaving a preview URL that deploys but never serves HTTPS. However, the suggested grep-based check is unreliable—Fly's documentation recommends JSON output with jq for scripting, not text parsing.
Use this approach instead:
🔧 Suggested fix
# Register custom domain with Fly.io (Cloudflare handles SSL)
- flyctl certs add "${APP_DOMAIN}" --app "${APP_NAME}" || true
+ # Check if cert already exists using JSON output (more reliable than grepping text)
+ if ! flyctl certs list --app "${APP_NAME}" -j | jq -e ".[] | select(.hostname == \"${APP_DOMAIN}\")" > /dev/null 2>&1; then
+ flyctl certs add "${APP_DOMAIN}" --app "${APP_NAME}"
+ fiThis uses JSON parsing instead of text grepping for robustness, and fails on genuine errors while safely skipping re-runs where the cert already exists.
🤖 Prompt for AI Agents
In @.github/workflows/pr_preview.yml around lines 292 - 299, The workflow
currently swallows errors by using `flyctl certs add "${APP_DOMAIN}" --app
"${APP_NAME}" || true`; instead, query Fly for existing certs using `flyctl
certs list --json` and `jq` to check if APP_DOMAIN already has a cert, skip
adding if it exists, otherwise run `flyctl certs add "${APP_DOMAIN}" --app
"${APP_NAME}"` and allow its non-zero exit code to fail the job; replace the `||
true` pattern with this JSON-based existence check and proper exit propagation
so auth/DNS/rate-limit errors are not ignored.
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.