fix(setup): fix seed 409 bug, README, docker-compose volumes, and deps#1678
fix(setup): fix seed 409 bug, README, docker-compose volumes, and deps#1678YogeshK34 wants to merge 6 commits into
Conversation
Signed-off-by: yogeshk34 <khutwadyogesh34@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR updates local setup documentation, Docker storage configuration, dependency ordering, and Prisma seed behavior for platform admin and Keycloak provisioning. ChangesLocal Setup and Seed Flow Updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant seed.ts
participant Keycloak
participant Prisma
seed.ts->>Keycloak: POST new user
alt 201 Created
Keycloak-->>seed.ts: Location header with user id
seed.ts->>Prisma: update platform admin with keycloakUserId and credentials
else 409 Conflict
Keycloak-->>seed.ts: existing user conflict
seed.ts->>Keycloak: lookup user by username
Keycloak-->>seed.ts: existing user id
seed.ts->>Prisma: sync existing keycloakUserId and credentials
else Other status
Keycloak-->>seed.ts: response text
seed.ts->>seed.ts: throw error with details
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
54-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe Postgres setup instructions currently conflict with
docker compose up -d.Step 2 creates
credebl-postgresmanually withcredebl/changeme, whiledocker-compose.ymlalso definescredebl-postgreswith different credentials (postgres/postgres). Running Step 7 (docker compose up -d) can cause container-name collision and/or auth mismatch.Suggested doc fix
-Then start it (along with other infrastructure) using Docker Compose: +Then start only NATS via Docker Compose: ```bash -docker compose up -d +docker compose up -d natsOr document a single Postgres path only (manual *or* compose) and keep credentials identical across both instructions. </details> Also applies to: 67-69, 220-221 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@README.mdaround lines 54 - 62, The README has conflicting Postgres setup
instructions where the manual docker run command for credebl-postgres uses
credentials credebl/changeme, but the docker-compose.yml defines the same
container with different credentials postgres/postgres. This causes
container-name collision and authentication mismatches when running docker
compose up -d after the manual setup. Either consolidate to a single setup
method (manual docker run OR docker-compose, but not both) and document only
that approach, or ensure the credentials are identical across all Postgres setup
instructions throughout the README so they can coexist without conflict. If
using docker-compose, update the docker compose up command to exclude the
postgres service if it was already created manually, or remove the manual setup
steps entirely in favor of the compose-based approach.</details> <!-- cr-comment:v1:c64ed4eaa18d46ea64493949 --> </blockquote></details> </blockquote></details>🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Inline comments: In `@README.md`: - Around line 75-91: The README.md documentation has an incorrect setup sequence where Prisma migrations and database seeding are instructed before dependencies are installed, which will fail because required tools like ts-node are not yet available. Add a new "pnpm install" step before the Prisma migration step at lines 75-91 (the anchor location), then renumber all subsequent steps throughout the document to account for this insertion. The same renumbering must be applied at the consolidated sibling locations in README.md at lines 191-197 and 225-230 to keep step numbering consistent across all sections of the documentation. - Around line 266-268: The fenced code block containing the Swagger URL in README.md is missing a language identifier, which violates markdownlint rule MD040. Add the language identifier "text" to the opening fence of the code block by changing the three backticks to three backticks followed by "text", so the block declares its language type as required by the markdown linting guidelines. --- Outside diff comments: In `@README.md`: - Around line 54-62: The README has conflicting Postgres setup instructions where the manual docker run command for credebl-postgres uses credentials credebl/changeme, but the docker-compose.yml defines the same container with different credentials postgres/postgres. This causes container-name collision and authentication mismatches when running docker compose up -d after the manual setup. Either consolidate to a single setup method (manual docker run OR docker-compose, but not both) and document only that approach, or ensure the credentials are identical across all Postgres setup instructions throughout the README so they can coexist without conflict. If using docker-compose, update the docker compose up command to exclude the postgres service if it was already created manually, or remove the manual setup steps entirely in favor of the compose-based approach.🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID:
2950deb7-df9d-4f58-9ddb-a56b4995cdd9⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml📒 Files selected for processing (4)
README.mddocker-compose.ymllibs/prisma-service/prisma/seed.tspackage.json
|
Hi @sagarkhole4 & @KambleSahil3 a quick heads up, there are some changes to be made from my end regarding the README.me flow, especially about the postgres container and it's inclusion in docker-compose.yml Before I proceed, I wanted to check if you have any feedback on existing changes |
| }, | ||
| "devDependencies": { | ||
| "@nestjs/cli": "catalog:", | ||
| "@nestjs/cli": "^11.0.23", |
There was a problem hiding this comment.
By bypassing the workspace catalog, this package version is now managed independently from the rest of the monorepo, which can
lead to version mismatches across different applications and libraries in the repository.
It should remain `"catalog:"` and instead be upgraded centrally in `pnpm-workspace.yaml`.
| ### • Install NestJS CLI | ||
| ```bash | ||
| npm i @nestjs/cli@latest | ||
| pnpm add -g @nestjs/cli |
There was a problem hiding this comment.
Project runs using the local
version in node_modules anyway, developers won't need to constantly upgrade their global CLI.
…mand Signed-off-by: yogeshk34 <khutwadyogesh34@gmail.com>
YogeshK34
force-pushed
the
fix/local-setup-docs-and-seed
branch
from
aa2d025 to
0b9c0bf
Compare
… condition Signed-off-by: yogeshk34 <khutwadyogesh34@gmail.com>
|
Updated both Also replaced the postgres startup command — it now starts via |
|
any update @RinkalBhojani |
|
Hi @YogeshK34, updates in seed.ts and docker-compose looks good. |
|
Thanks for pointing that out @ankita-p17! I went through the setup docs & below are my observations and what I plan to change: Postgres: Docs walk users through manual docker compose up -d postgresCleaner, and it also avoids the warning "Docker Compose also defines a service named Keycloak: Happy to redirect users to the docs for the heavy lifting, but a couple of things worth flagging:
Here's the plan: start Keycloak with my corrected command (right network, pinned version, named container), then redirect to docs for the realm and client setup steps and remove the existing client/realm creation from the README since docs already cover that. Let me know if you'd like any further changes, otherwise I'll go ahead with this! |
Hi @YogeshK34, this plan looks good, just one thing, we are intentionally defining keycloak version since latest may have some breaking changes, so please keep the version as is. |
…date README Signed-off-by: yogeshk34 <khutwadyogesh34@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/prisma-service/prisma/seed.ts (1)
251-262: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck the
user_org_rolesrow, not just prerequisite records.Line 251 skips creating the platform admin role whenever the user/org/role all exist, which is the normal successful seed path. If any prerequisite is missing, the
elsebranch then dereferences.idon a missing value. Validate prerequisites first, then query/create the join row.🐛 Proposed fix
- if (userId && orgId && orgRoleId) { + if (!userId || !orgId || !orgRoleId) { + throw new Error('Missing prerequisite platform admin user, organization, or org role'); + } + + const existingUserOrgRole = await prisma.user_org_roles.findFirst({ + where: { + userId: userId.id, + orgRoleId: orgRoleId.id, + orgId: orgId.id + } + }); + + if (existingUserOrgRole) { logger.log('Already seeding in org_roles'); } else { const platformOrganization = await prisma.user_org_roles.create({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/prisma-service/prisma/seed.ts` around lines 251 - 262, The seeding logic in the `prisma.user_org_roles.create` block is checking only whether `userId`, `orgId`, and `orgRoleId` exist, which incorrectly skips the normal create path and can still dereference missing `.id` values in the `else` branch. Update the `seed.ts` logic to first validate that the prerequisite `user`, `org`, and `role` records are present, then explicitly query the `user_org_roles` join row to see whether the platform admin relationship already exists before creating it. Use the `prisma.user_org_roles` lookup/create flow and the existing `logger.log` statements to keep the behavior aligned with the seed helpers.
🧹 Nitpick comments (2)
README.md (2)
130-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix blockquote blank lines flagged by markdownlint (MD028).
Blank lines inside consecutive
>blockquotes split them into separate blockquotes rather than one continuous block, per the linter warnings at these lines.As per static analysis hints, `markdownlint-cli2` flags "Blank line inside blockquote (MD028, no-blanks-blockquote)" at lines 132 and 134.Suggested doc fix
-> For remaining Keycloak env variables (`KEYCLOAK_REALM`, `KEYCLOAK_MASTER_REALM`, `KEYCLOAK_MANAGEMENT_CLIENT_ID`, `KEYCLOAK_MANAGEMENT_CLIENT_SECRET`, `ADMIN_KEYCLOAK_ID`, `ADMIN_KEYCLOAK_SECRET`), refer to the official docs linked above. - -> **Note:** If you want to log into Studio UI, follow the optional user creation step in docs — use the **same email** as `PLATFORM_ADMIN_EMAIL` when adding the user in Keycloak, otherwise authentication will fail. - -> ⚠️ All Keycloak env variables must be fully configured before running the seed — the seed script connects to Keycloak directly. +> For remaining Keycloak env variables (`KEYCLOAK_REALM`, `KEYCLOAK_MASTER_REALM`, `KEYCLOAK_MANAGEMENT_CLIENT_ID`, `KEYCLOAK_MANAGEMENT_CLIENT_SECRET`, `ADMIN_KEYCLOAK_ID`, `ADMIN_KEYCLOAK_SECRET`), refer to the official docs linked above. +> +> **Note:** If you want to log into Studio UI, follow the optional user creation step in docs — use the **same email** as `PLATFORM_ADMIN_EMAIL` when adding the user in Keycloak, otherwise authentication will fail. +> +> ⚠️ All Keycloak env variables must be fully configured before running the seed — the seed script connects to Keycloak directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 130 - 136, The README blockquote formatting is triggering markdownlint MD028 because blank lines split the consecutive quoted notes into separate blockquotes. Update the affected quoted text in README.md so the `>` lines are contiguous without blank lines between them, keeping the same content but preserving a single continuous blockquote across the note section.Source: Linters/SAST tools
148-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrailing whitespace on horizontal rule (MD035).
As per static analysis hints, `markdownlint-cli2` flags "Horizontal rule style Expected: ---; Actual: --- " at line 148.Suggested doc fix
---- +---🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 148, The markdown horizontal rule in the README contains trailing whitespace that triggers MD035. Update the horizontal rule near the referenced section so it is exactly the expected style with no extra spaces, using the surrounding README content as the locator.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/prisma-service/prisma/seed.ts`:
- Around line 701-716: The user creation flow in the user-to-Keycloak request
currently falls back to an empty credentials array when user.password is
missing, which can mask a bad decrypt and create an admin user without login
access. Update the Keycloak user provisioning path in the seed logic around the
credentials construction and fetch call to validate that the decrypted admin
password is present and non-empty before posting; if it is empty, throw
immediately instead of sending the request. Use the existing user/password
handling in the seed.ts user creation helper to locate the fix.
---
Outside diff comments:
In `@libs/prisma-service/prisma/seed.ts`:
- Around line 251-262: The seeding logic in the `prisma.user_org_roles.create`
block is checking only whether `userId`, `orgId`, and `orgRoleId` exist, which
incorrectly skips the normal create path and can still dereference missing `.id`
values in the `else` branch. Update the `seed.ts` logic to first validate that
the prerequisite `user`, `org`, and `role` records are present, then explicitly
query the `user_org_roles` join row to see whether the platform admin
relationship already exists before creating it. Use the `prisma.user_org_roles`
lookup/create flow and the existing `logger.log` statements to keep the behavior
aligned with the seed helpers.
---
Nitpick comments:
In `@README.md`:
- Around line 130-136: The README blockquote formatting is triggering
markdownlint MD028 because blank lines split the consecutive quoted notes into
separate blockquotes. Update the affected quoted text in README.md so the `>`
lines are contiguous without blank lines between them, keeping the same content
but preserving a single continuous blockquote across the note section.
- Line 148: The markdown horizontal rule in the README contains trailing
whitespace that triggers MD035. Update the horizontal rule near the referenced
section so it is exactly the expected style with no extra spaces, using the
surrounding README content as the locator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: beac7f0f-14df-449e-a3f1-d4fb4b98fd08
📒 Files selected for processing (2)
README.mdlibs/prisma-service/prisma/seed.ts
…roles Signed-off-by: yogeshk34 <khutwadyogesh34@gmail.com>
Signed-off-by: yogeshk34 <khutwadyogesh34@gmail.com>
|
|
Done @ankita-p17 |
4 participants
Summary
Fixes a series of undocumented and broken steps in the GitHub repo-based local setup. These issues were discovered during a full end-to-end setup from scratch. Most developers will hit all of them in sequence and have no documented path to resolution.
This PR is scoped to the platform repo only. Studio works correctly once the platform is properly configured.
Changes
README.md— Full rewriteThe existing README was missing critical setup steps. All issues below were verified against the actual codebase before documenting:
docker runused hardcoded credentials not tied toDATABASE_URL.env, note they must be consistentprisma migrateorder was wrong or absent — seeding ran before tables existedadminClient+credeblClient, configure Service Account Roles for each, copy secrets to.envnpm install— this project usespnpmpnpm; added prominent warningmanage-users,view-users,query-users,manage-realm) for both clientsKEYCLOAK_DOMAINwhen services run in Dockerlocalhostdoes not resolve to the host from inside a container; must use LAN IPplatform_defaultnetwork--network platform_defaultto Keycloakdocker runexampleseed.ts— Bug fix: 409 Conflict early-returncreateKeycloakUser()returned early when the Keycloak user already existed (HTTP 409), without updatingkeycloakUserId,clientId, orclientSecretin the DB. On any re-seed (or when the user was created outside the seed script), this left the user record in a broken state wherekeycloakUserId = "", causing login to silently fall through to the Supabase path — which then threwBadRequestException('Invalid Credentials')on the placeholder client secret.Fix: On 409, query the Keycloak Admin API (
GET /users?username=...&exact=true) to retrieve the existing user's ID, then proceed with the same DB update as a fresh creation.docker-compose.yml— Missing volume declarationThe
postgresservice mountedplatform-volumebutplatform-volumewas never declared in the top-levelvolumesblock. This causeddocker compose upto fail with:Added the missing declaration.
package.json+pnpm-lock.yaml— Dependency cleanupSorted and deduplicated dependencies that had accumulated out of alphabetical order (
form-data,handlebars,protobufjs). Pinned@nestjs/clito^11.0.23instead ofcatalog:to avoid resolution issues in environments without a pnpm catalog configured.Testing
prisma migrate deploy→prisma db seedsequence verified workingdocker compose upwith the fixeddocker-compose.yml: ✅ no volume errorsWhat's not in this PR
credebl-master-table.json— intentionally excluded; contains local/personal config values that must remain as placeholders for each developer's environmentSummary by CodeRabbit
Release Notes
.env→ PostgreSQL → migrations → seed), and clearer Keycloak networking and URL guidance for Docker.platform-volumeand reordered dependencies inpackage.jsonfor consistency.