fix: point shared-deployments to feature branch in deploy.yaml#2
fix: point shared-deployments to feature branch in deploy.yaml#2Matanya7491 wants to merge 12 commits into
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
| // Must run before Prisma initialises | ||
| resolveDATABASE_URL(); | ||
| resolveConnURLs(); | ||
| resolveRedisURLs(); |
There was a problem hiding this comment.
CONNECTION_MAP is snapshotted at PostgresCommand module load, so deriving CONN_LINK_API_URL/CONN_LINK_API_RO_URL after AppModule import can make connection: 'link_api'/'link_api_ro' miss and hit Unknown connection; can we move the env derivation before the AppModule require (or into a bootstrap module required first)?
Finding type: Breaking Changes | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/backend/src/main.ts around lines 1-66, fix the initialization order so
resolveDATABASE_URL() and resolveConnURLs() run before importing AppModule (and before
any modules/commands that snapshot process.env at module load). Refactor by moving the
env-derivation functions and their invocations above the AppModule import, or by
extracting them into a new tiny bootstrap file that main.ts imports first, then
importing AppModule afterwards. Ensure PostgresCommand (and any similar command that
builds CONNECTION_MAP at import time) observes CONN_LINK_API_URL and
CONN_LINK_API_RO_URL set during startup, so seeded postgres flows don’t fail with
“Unknown connection” when those env vars are derived internally.
| const match = dbUrl.match(/\/([^/?]+)(\?|$)/); | ||
| const dbName = match?.[1] ?? 'validation_platform'; |
There was a problem hiding this comment.
DATABASE_URL without a /dbname path is valid for libpq but this ?? 'validation_platform' default can make ensureDatabase() connect/migrate the wrong DB; should we fail fast when the db name is missing, or parse/derive the actual database name from the URL?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/backend/src/main.ts around lines 73-107, inside ensureDatabase(), the code derives
dbName using dbUrl.match(/\/([^/?]+)(\?|$)/) and then defaults with match?.[1] ??
'validation_platform'. This silently chooses the wrong database when DATABASE_URL is a
valid Postgres URI without a /dbname path. Refactor ensureDatabase() to explicitly parse
the database name from DATABASE_URL (prefer URL/authority parsing), and if the dbname
component is missing/ambiguous, fail fast (throw or exit) with a clear error instead of
defaulting; then ensure adminUrl is built only from the correctly extracted dbName.
…grate on startup Co-authored-by: Cursor <cursoragent@cursor.com>
| # Frontend — Next.js standalone output | ||
| COPY --from=web-build /usr/src/app/apps/web/.next ./apps/web/.next | ||
| COPY --from=web-build /usr/src/app/apps/web/.next/standalone ./apps/web/.next/standalone | ||
| COPY --from=web-build /usr/src/app/apps/web/.next/static ./apps/web/.next/static | ||
| COPY --from=web-build /usr/src/app/apps/web/public ./apps/web/public |
There was a problem hiding this comment.
Since the Dockerfile now copies Next standalone into ./apps/web/.next/standalone, should we confirm the final image CMD actually starts node apps/web/.next/standalone/server.js (otherwise the copied server.js never runs), or split web into a dedicated container?
Finding type: Breaking Changes | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In Dockerfile around
lines 49-52 where you copy the Next.js standalone output (copying `.next/standalone` and
`.next/static` into `./apps/web/.next/...`), ensure the final runtime stage actually
starts the Next standalone server instead of only the backend. Update the final
stage’s CMD/ENTRYPOINT (outside this snippet) to run `node
apps/web/.next/standalone/server.js` (or ensure the correct working directory and paths)
and verify the web server listens on the configured PORT. If the project should not run
both services in one image, refactor by splitting into separate Dockerfiles/targets for
backend and web so each image starts its own entrypoint.
Co-authored-by: Cursor <cursoragent@cursor.com>
| FROM dependencies AS prod-deps | ||
| RUN npm prune --omit=dev --workspace=apps/backend |
There was a problem hiding this comment.
npm prune --omit=dev --workspace=apps/backend only targets that workspace, so later root-level npm ci plus copying /usr/src/app/node_modules can leave the final image with root-hoisted (incl dev) workspace deps. Can we also prune the root (or avoid copying the root tree) before building the runtime image?
Finding type: Logical Bugs | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In Dockerfile around
lines 30-31 (the `prod-deps` stage running `npm prune --omit=dev
--workspace=apps/backend`), fix the logic so the runtime image is truly
backend-only/prod-only. Currently the stage only prunes the backend workspace, but later
lines 38-39 copy `/usr/src/app/node_modules` into the production image, which came from
the root `npm ci` and can still include root/hoisted workspace links and dev
dependencies. Refactor the stages so either (a) you also prune the root project for
production deps (not just the backend workspace), and ensure workspace links like
`node_modules/web` are removed, or (b) you stop copying `/usr/src/app/node_modules` into
the final image and only copy `/usr/src/app/apps/backend/node_modules` plus the minimal
runtime files needed. After the change, verify the final image `node_modules` contains
only backend production dependencies (no web workspace link and no dev packages).
…ement) Co-authored-by: Cursor <cursoragent@cursor.com>
package-lock.json was missing @bringits-validation-platform/backend workspace entry, causing `npm ci` to fail with EUSAGE mismatch. Regenerated lockfile with `npm install`. Simplified Dockerfile to plain `npm ci --ignore-scripts` (no --include-workspace-root needed). Co-authored-by: Cursor <cursoragent@cursor.com>
| COPY --from=build-web /usr/src/app/apps/web/.next/standalone ./apps/web/.next/standalone | ||
| COPY --from=build-web /usr/src/app/apps/web/.next/static ./apps/web/.next/static | ||
| COPY --from=build-web /usr/src/app/apps/web/public ./apps/web/public |
There was a problem hiding this comment.
With output: 'standalone', server.js expects assets under .next/standalone/public and .next/standalone/.next/static, but this Dockerfile copies from apps/web/public and apps/web/.next/static; can we copy into the standalone tree paths so the server can actually find them?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In Dockerfile around
lines 37-39 in the production stage (the `COPY --from=build-web` commands for the
Next.js output), fix the path mismatch for Next.js `output: 'standalone'`. Right now
`public` and `.next/static` are copied to `apps/web/public` and `apps/web/.next/static`,
but the standalone `server.js` expects them under the standalone tree at
`apps/web/.next/standalone/public` and `apps/web/.next/standalone/.next/static`. Update
these COPY destinations so both `public` and static assets are placed inside
`apps/web/.next/standalone/` (and avoid copying them only to sibling directories), then
verify the standalone server can resolve them at runtime.
There was a problem hiding this comment.
Commit db00321 addressed this comment by copying the built Next.js standalone tree (# .next/standalone) into the production image so the server can resolve its expected public and static assets paths, rather than only exposing the top-level public/.next/static directories.
npx --prefix changes package resolution path but not cwd, causing Prisma to fail to locate schema.prisma. Use node_modules/.bin/prisma directly with --schema=apps/backend/prisma/schema.prisma instead. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…or migrate Co-authored-by: Cursor <cursoragent@cursor.com>
| } catch (err: any) { | ||
| console.error('[bootstrap] Migration failed:', err.message); | ||
| } |
There was a problem hiding this comment.
Since prisma migrate deploy failures are only logged here, bootstrap() keeps running NestFactory.create()/listen() even if the schema didn’t apply, so Prisma-backed routes like GET /api/runs can serve against missing/stale tables. Can we exit or block readiness when migrations fail?
Finding type: Breaking Changes | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/backend/src/main.ts around lines 118-120 where you run `prisma migrate deploy`
(before calling `bootstrap()`), the migration error is being caught and only logged,
allowing `bootstrap()` / `NestFactory.create()` / `listen()` to proceed even if the
schema was not applied. Refactor so migration failure causes the process to stop (e.g.,
rethrow the error or call `process.exit(1)` after logging) so the service doesn’t
start and serve Prisma-backed routes with a missing/stale schema. Ensure this failure
prevents both app initialization and request handling when `prisma migrate deploy`
fails.
User description
Summary
Updates
pipelines/deploy.yamlto reference the correct branch inshared-deploymentswhile the Kubernetes values PR is pending merge.Change:
This tells Azure DevOps to use the
feature/add-bringits-validation-platformbranch ofshared-deployments(where thevalues.yamllives) instead ofmain.After shared-deployments PR #9251 is merged, this
refshould be removed so the pipeline usesmain.Related
Made with Cursor
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Enable the backend’s bootstrap sequence to expose health/command-type metadata via
MetaController, resolve runtime URLs, ensure the validation_platform database exists, and run Prisma migrations beforeNestFactorystarts so the API is resilient on new deployments. Update the Docker/Next.js build pipeline, workspace metadata, and shared-deployments reference so the production container ships the compiled backend/web artifacts alongside the temporary feature branch values required for the bringits deployment flow.MetaControllerat the root to servehealthandmeta/command-types, by resolving link-db and Redis URLs from cloud secrets, and by ensuring the validation_platform database is created and migrated before NestJS listens so the service starts with predictable dependencies.Modified files (2)
Latest Contributors(1)
package-lock.json, and pointingpipelines/deploy.yamlat the shared-deployments feature branch so the production image contains the compiled backend/web assets plus the temporary values from bringits deployments.Modified files (4)
Latest Contributors(1)