Skip to content

feat: automatically create review apps#7450

Draft
nickskalkin wants to merge 2 commits intomainfrom
nickskalkin/automatically-create-review-apps
Draft

feat: automatically create review apps#7450
nickskalkin wants to merge 2 commits intomainfrom
nickskalkin/automatically-create-review-apps

Conversation

@nickskalkin
Copy link
Contributor

Note

This is still WIP. I haven't tested the changes yet

I really like how easy it is to create a review app in Force, and I’d love to have the same experience in Metaphysics. I borrowed the relevant CircleCI config sections and scripts to make it work.

Now pushing to any branch with the review-app- prefix triggers workflows to provision and configure review apps in Kubernetes and Cloudflare DNS.

@nickskalkin nickskalkin marked this pull request as draft February 13, 2026 16:01
@claude
Copy link

claude bot commented Feb 13, 2026

Summary

This PR adds automated review app creation for Metaphysics. When developers push to a branch with the review-app- prefix, CircleCI automatically provisions a Kubernetes deployment and creates a corresponding Cloudflare DNS subdomain, making the review app accessible at metaphysics-{branch-name}.artsy.net.

Issues Found

🟡 Important: Missing error handling could cause DNS creation failures

In scripts/create-review-app-subdomain.js:

  • File: scripts/create-review-app-subdomain.js:40-42
  • Code:
} catch (error) {
  console.error(error)
}
  • Problem: The script only logs errors but doesn't exit with a non-zero status. This means CircleCI will mark this step as successful even if DNS creation fails. The review app would be deployed but inaccessible.
  • Suggested fix: Add process.exit(1) after logging the error to properly signal failure to CircleCI.

🟡 Important: Missing environment variable validation

In scripts/create-review-app-subdomain.js:

  • File: scripts/create-review-app-subdomain.js:2-4
  • Code:
const CLOUDFLARE_ZONE_ID = process.env.CLOUDFLARE_ZONE_ID
const CLOUDFLARE_API_TOKEN = process.env.CLOUDFLARE_API_TOKEN
const BRANCH_NAME = process.env.CIRCLE_BRANCH
  • Problem: The script doesn't validate that these required environment variables are present before attempting to use them. This could lead to confusing errors when the API call is made with undefined values.
  • Suggested fix: Add validation at the start of the script to check these variables exist and fail early with a clear error message.

🟢 Suggestion: Potential DNS record conflicts

In scripts/create-review-app-subdomain.js:

  • File: scripts/create-review-app-subdomain.js:7
  • Code: const dnsSubdomain = BRANCH_NAME.replace("review-app-", "")
  • Observation: The script assumes creating a new record but doesn't handle the case where a CNAME already exists for that subdomain. While Cloudflare might handle this gracefully, it would be cleaner to check if the record exists first and update it if needed, especially for long-lived review apps that get redeployed.

🟢 Suggestion: Review app namespace cleanup

The scripts create review apps but there's no automated cleanup process mentioned. This could lead to:

  • Accumulation of unused Kubernetes namespaces
  • Lingering DNS records in Cloudflare
  • Resource consumption in the staging cluster

Consider adding a cleanup mechanism (manual script or automated based on branch deletion/age).

Areas Reviewed

Architecture & Design

The implementation follows a clean separation of concerns with dedicated scripts for each operation. The integration with CircleCI's branch filtering is elegant and the use of hokusai commands is appropriate.

Security

  • ✅ Uses CircleCI contexts for sensitive credentials (cloudflare, hokusai)
  • ✅ DNS records are properly proxied through Cloudflare
  • ✅ Review apps use the staging infrastructure, not production

Performance

No significant concerns. The sequential execution of review app creation and DNS setup is appropriate given the dependency.

Testing

The scripts lack automated tests, but given their nature as deployment scripts that interact with external services, this is understandable. Manual testing would be appropriate.

Questions for Author

  1. How should developers clean up review apps when they're no longer needed? Is this a manual process using hokusai review_app delete?

  2. What happens if someone creates a branch like review-app-main or uses other reserved/problematic names? Should there be validation for the extracted subdomain?

  3. The nginx-staging-2025.artsy.net CNAME target is hardcoded. Is this value expected to change, and if so, should it be configurable?

  4. Will there be any resource limits or quotas for the number of concurrent review apps to prevent cluster resource exhaustion?

Comment on lines +40 to +42
} catch (error) {
console.error(error)
}
Copy link

Choose a reason for hiding this comment

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

The script should exit with a non-zero status when an error occurs to properly signal failure to CircleCI:

Suggested change
} catch (error) {
console.error(error)
}
} catch (error) {
console.error(error)
process.exit(1)
}

Without this, CircleCI will mark the step as successful even if DNS creation fails, leaving the review app inaccessible.

Comment on lines +1 to +4
const CLOUDFLARE_API_URL = "https://api.cloudflare.com/client/v4"
const CLOUDFLARE_ZONE_ID = process.env.CLOUDFLARE_ZONE_ID
const CLOUDFLARE_API_TOKEN = process.env.CLOUDFLARE_API_TOKEN
const BRANCH_NAME = process.env.CIRCLE_BRANCH
Copy link

Choose a reason for hiding this comment

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

Consider adding validation for required environment variables at the start of the script to fail early with clear error messages:

const CLOUDFLARE_API_URL = "https://api.cloudflare.com/client/v4"
const CLOUDFLARE_ZONE_ID = process.env.CLOUDFLARE_ZONE_ID
const CLOUDFLARE_API_TOKEN = process.env.CLOUDFLARE_API_TOKEN
const BRANCH_NAME = process.env.CIRCLE_BRANCH

// Validate required environment variables
if (!CLOUDFLARE_ZONE_ID || !CLOUDFLARE_API_TOKEN || !BRANCH_NAME) {
  console.error("Missing required environment variables: CLOUDFLARE_ZONE_ID, CLOUDFLARE_API_TOKEN, or CIRCLE_BRANCH")
  process.exit(1)
}

This prevents confusing API errors when these variables are undefined.

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.

1 participant