refactor(admin-roles): migrate from server by shikanime · Pull Request #2371 · cloud-pi-native/console

shikanime · 2026-07-27T09:23:05Z

Change-Id: I6e2bf93ddac464c88a82683dc6e6b0846a6a6964

Issues liées

Issues numéro: #2204
Reopened Pull Request: #2205

Quel est le comportement actuel ?

Quel est le nouveau comportement ?

Cette PR introduit-elle un breaking change ?

Autres informations

github-actions · 2026-07-27T09:59:15Z

🤖 Hey !

The security scan report for the current pull request is available here.

github-actions · 2026-07-27T10:09:01Z

🤖 Hey !

The security scan report for the current pull request is available here.

github-actions · 2026-07-27T10:25:47Z

🤖 Hey !

The security scan report for the current pull request is available here.

shikanime · 2026-07-27T13:45:35Z

+        })
+      }
+
+      if (positionsAvailable.length && positionsAvailable.length !== dbRoles.length) {


Dead position-integrity guard (parity note): positionsAvailable is only populated for roles found in dbRoles, so its length always equals the number of matched DB roles. The !== dbRoles.length branch is effectively unreachable on the normal partial-patch path. Reproduces legacy behavior (business.ts:39) — harmless, but worth a comment or removal so it is not mistaken for an active invariant.

Ben, surtout, il est où le test unitaire qui valide/invalide ce if ?

shikanime · 2026-07-27T13:45:36Z

+        select: { id: true, adminRoleIds: true },
+      })
+
+      await this.eventEmitter.emitAsync('adminRole.delete', {


adminRole.delete is emitted inside the transaction (before the row is deleted at :185). Legacy does the same (business.ts:94), so this is parity-preserving — but if a future @OnEvent(adminRole.delete) consumer reads the role row, ordering matters. Confirm intended.

D'ailleurs faudrait clarifier si on veut adminRole.delete (action de supprimer) ou adminRole.deleted (résultat de la suppression).

En termes d'intention et de conséquences, ça peut effectivement être problématique.

On a fait une carto des flux d'évènement d'ailleurs ? On se fait plutôt ça dans le ticket de carto des plugins ( #2181 )

shikanime · 2026-07-27T13:45:37Z

+import type { AdminRoleService } from './admin-role.service'
+import type { CreateAdminRoleBody, PatchAdminRolesBody } from './admin-role.utils'
+
+export type AdminRoleContract = Parameters<AdminRoleService['patch']>[0][number]


Unused testing-utils exports: AdminRoleContract, AdminRoleResponse, and makeAdminRoleMember (lines 5, 6, 27) have no importers — the spec only uses makeAdminRole / makeCreateAdminRoleBody. Drop them or wire them into the spec to avoid dead surface.

shikanime · 2026-07-27T13:45:38Z

+  @UseGuards(UserGuard)
+  // TODO: ListRoles is intentionally not protected by admin permission because of
+  // certain behaviours of the legacy client
+  // @RequireAdminPermission('ListRoles')


ListRoles intentionally unguarded (legacy-client behavior) — matches the legacy router. Low priority: note which legacy client path depends on this so the TODO can be closed later.

Ça demande un ticket, à mon avis, car c'est suffisament atomique, comme considération 🙂

shikanime

Verdict: REQUEST CHANGES

Migration of admin-roles from the legacy Fastify server into server-nestjs is well-structured (typecheck clean, admin-role.service.spec.ts passes, eslint clean). One blocker must be resolved before the legacy router is cut over.

Blocker — emitted adminRole.* events are never consumed (silent Keycloak/GitLab sync regression).
The service emits adminRole.upsert / adminRole.delete (service.ts:45, :124, :169) via EventEmitter2, but no @OnEvent('adminRole.upsert'|'adminRole.delete') handler exists in server-nestjs and @cpn-console/hooks is not imported by the module. In the legacy server, those same flows call hook.adminRole.upsert / hook.adminRole.delete (business.ts:42,66,94), which drive the Keycloak OIDC-group sync and GitLab admin/auditor group sync. Every other migrated entity bridges its event into the plugin hook system via an @OnEvent handler (e.g. keycloak.service.ts:28, gitlab.service.ts:63). Without that bridge, once the legacy route is removed, admin-role CRUD will silently stop syncing member groups. Add the @OnEvent bridge handlers (mirroring the project bridge) or explicitly defer with a tracked follow-up before cutover.

Warnings (parity-preserving, confirm): dead patch position-integrity guard (service.ts:92) — only reachable in the legacy-incompatible all-roles path; adminRole.delete emitted before the row delete inside the transaction (service.ts:169 vs :185) — legacy does the same.

Minor: unused testing-utils exports (AdminRoleContract, AdminRoleResponse, makeAdminRoleMember in admin-role-testing.utils.ts:5,6,27).

Full detail in the inline comments.

github-actions · 2026-07-27T14:42:46Z

🤖 Hey !

The security scan report for the current pull request is available here.

github-actions · 2026-07-28T13:35:39Z

🤖 Hey !

The security scan report for the current pull request is available here.

StephaneTrebel

Rien qui me choque, mais les questions évoquées me semblent pertinentes

StephaneTrebel · 2026-07-30T08:18:23Z

+  @UseGuards(UserGuard)
+  // TODO: ListRoles is intentionally not protected by admin permission because of
+  // certain behaviours of the legacy client
+  // @RequireAdminPermission('ListRoles')


Ça demande un ticket, à mon avis, car c'est suffisament atomique, comme considération 🙂

StephaneTrebel · 2026-07-30T08:20:43Z

+        })
+      }
+
+      if (positionsAvailable.length && positionsAvailable.length !== dbRoles.length) {


Ben, surtout, il est où le test unitaire qui valide/invalide ce if ?

StephaneTrebel · 2026-07-30T08:23:32Z

+        select: { id: true, adminRoleIds: true },
+      })
+
+      await this.eventEmitter.emitAsync('adminRole.delete', {


D'ailleurs faudrait clarifier si on veut adminRole.delete (action de supprimer) ou adminRole.deleted (résultat de la suppression).

En termes d'intention et de conséquences, ça peut effectivement être problématique.

On a fait une carto des flux d'évènement d'ailleurs ? On se fait plutôt ça dans le ticket de carto des plugins ( #2181 )

shikanime · 2026-08-04T14:01:43Z

Review: PR #2371 — verdict REQUEST CHANGES

I reviewed the actual diff (34 commits / 206 files). CI is green (unit-tests, lint, build, SonarQube, Trivy, CodeQL all pass) and the branch is mergeable with no conflicts.

The quality of the code is high: the deployment valueSources feature uses a clean parse-don't-validate boundary, the config→nestjs migration is coherent, and the admin-role migration is faithful to legacy. But a few issues should be fixed before merge. (Note: GitHub won't let me set a formal "Request changes" state on my own PR, so this is posted as a comment — please treat the blockers as required.)

Blockers

Misleading title/scope — titled refactor(admin-roles): migrate from server but actually covers deployment value-sources, the full config→nestjs migration, conditional module activation, health-check refactors, CI changes, and admin-role. Either split into stacked PRs or correct the title/description.
deleteAllDeploymentsByProjectId unhandled rejection — apps/server-nestjs/src/modules/deployment/deployment.service.ts:88-90 calls emitProjectEvent with no .catch, while the 3 sibling methods route through reconcileProject() which catches. Add a .catch for parity.
admin-role patch untested — only create has a spec (admin-role.service.spec.ts). patch (position guard), delete, list, memberCounts are uncovered. Add at least one patch spec for the position-coherence branch.

Warnings (follow-up)

Position guard quirk carried from legacy (admin-role.service.ts:92 ≈ legacy business.ts:39): a partial PATCH including position on a subset throws 400. Faithful, but worth a follow-up issue to relax to "unique/contiguous among provided roles."
listAdminRoles intentionally unauthenticated (admin-role.controller.ts:18 TODO) — matches legacy + client need; resolve the TODO later.

Nits

getDotenvPaths() (utils/dotenv.utils.ts) returns relative filenames; assumes process.cwd() is the app root. Fine today, noted.

shikanime · 2026-08-06T12:21:27Z

Review: #2371 — refactor(admin-roles): migrate from server

Verdict: REQUEST CHANGES (reviewer agent) — blocker still open since b2642e08.

[blocker] Emitted adminRole.upsert / adminRole.delete events (admin-role.service.ts:45, :124, :169 via EventEmitter2) are never consumed. There is no @OnEvent('adminRole.*') handler and admin-role.module.ts does not import @cpn-console/hooks. server-nestjs has no generic event→hook bridge (only project.upsert has explicit per-entity handlers). In the legacy server, adminRole.upsert/delete drive the Keycloak OIDC-group + GitLab admin/auditor group sync. Without the bridge, admin-role CRUD will silently stop syncing member groups after legacy-route cutover. Add the @OnEvent bridge handlers (mirror project.service) or defer with a tracked follow-up before cutover.

Warnings (parity-preserving): dead patch position-integrity guard (admin-role.service.ts:92); adminRole.delete emitted before the row delete inside the transaction (legacy does the same). Minor: unused testing-utils exports.

Cannot approve until the blocker is resolved.

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I00a18a853330301a735cdfcf3fb6955a6a6a6964

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I6e2bf93ddac464c88a82683dc6e6b0846a6a6964

cloud-pi-native-sonarqube · 2026-08-07T15:29:25Z

Quality Gate passed

Issues

Measures

Project ID: cloud-pi-native_console_AYsa46O31ebDtzs-pYyZ

View in SonarQube

shikanime added this to the 9.24.0 milestone Jul 27, 2026

shikanime added the enhancement New feature or request label Jul 27, 2026

shikanime self-assigned this Jul 27, 2026

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from e25c2f2 to 3218292 Compare July 27, 2026 09:54

github-actions Bot added the built label Jul 27, 2026

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from 3218292 to 3044f04 Compare July 27, 2026 10:04

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from 3044f04 to b2642e0 Compare July 27, 2026 10:10

shikanime requested review from KepoParis and StephaneTrebel July 27, 2026 10:11

shikanime commented Jul 27, 2026

View reviewed changes

Comment thread apps/server-nestjs/src/modules/admin-role/admin-role.service.ts

shikanime commented Jul 27, 2026

View reviewed changes

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from b2642e0 to b5725e4 Compare July 27, 2026 14:36

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from b5725e4 to de5e877 Compare July 28, 2026 13:27

StephaneTrebel reviewed Jul 30, 2026

View reviewed changes

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from de5e877 to 046ee74 Compare August 3, 2026 09:05

github-code-quality Bot found potential problems Aug 3, 2026

View reviewed changes

Comment thread apps/server-nestjs/src/main.module.ts Dismissed

shikanime added this to Cloud Pi Native Aug 4, 2026

shikanime moved this to Backlog in Cloud Pi Native Aug 4, 2026

shikanime moved this from Backlog to In progress in Cloud Pi Native Aug 4, 2026

shikanime moved this from In progress to In review in Cloud Pi Native Aug 4, 2026

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch 2 times, most recently from ca7cef4 to 273f574 Compare August 5, 2026 15:15

shikanime requested a review from StephaneTrebel August 6, 2026 14:38

shikanime marked this pull request as draft August 6, 2026 14:38

shikanime modified the milestones: 9.24.0, 9.25.0 Aug 7, 2026

shikanime mentioned this pull request Aug 7, 2026

💡 [REQUEST] - PluginManager : factoriser la plomberie commune des plugins (Log, Cron, event, autoSync/Suspend, flags) #2323

Open

5 tasks

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from 273f574 to 986437f Compare August 7, 2026 13:18

shikanime and others added 2 commits August 7, 2026 16:41

refactor(shared): add token pair generation and expiration schema

cc12cc9

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I00a18a853330301a735cdfcf3fb6955a6a6a6964

refactor(admin-roles): migrate from server

c491b64

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I6e2bf93ddac464c88a82683dc6e6b0846a6a6964

shikanime force-pushed the shikanime/push-tlxokqwmmpnv branch from 986437f to c491b64 Compare August 7, 2026 15:23

Uh oh!

Conversation

shikanime commented Jul 27, 2026

Issues liées

Quel est le comportement actuel ?

Quel est le nouveau comportement ?

Cette PR introduit-elle un breaking change ?

Autres informations

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shikanime left a comment

Choose a reason for hiding this comment

Verdict: REQUEST CHANGES

Uh oh!

github-actions Bot commented Jul 27, 2026

Uh oh!

github-actions Bot commented Jul 28, 2026

Uh oh!

StephaneTrebel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shikanime commented Aug 4, 2026

Review: PR #2371 — verdict REQUEST CHANGES

Blockers

Warnings (follow-up)

Nits

Uh oh!

shikanime commented Aug 6, 2026

Review: #2371 — refactor(admin-roles): migrate from server

Uh oh!

cloud-pi-native-sonarqube Bot commented Aug 7, 2026

Quality Gate passed

Issues

Measures

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants