refactor(server-nestjs): configuration to module definition - #2365
refactor(server-nestjs): configuration to module definition#2365shikanime wants to merge 2 commits into
Conversation
|
🤖 Hey ! The security scan report for the current pull request is available here. |
Review: PR #2365 — refactor(server-nestjs): configuration to module definitionReviewed branch Verdict: REQUEST CHANGESThe architecture is correct and idiomatic (ConfigurableModuleBuilder, gated modules via Blocker B1 —
|
df02c68 to
9b067ad
Compare
|
🤖 Hey ! The security scan report for the current pull request is available here. |
9b067ad to
dd57156
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
dd57156 to
cc10fe6
Compare
shikanime
left a comment
shikanime
left a comment
There was a problem hiding this comment.
Review: #2365 — refactor(server-nestjs): configuration to module definition
Verdict: REQUEST CHANGES (CI red + correctness bug)
Findings
[blocker] CI is failing at head dd57156 — check-runs unit-tests / Unit tests, lint / Lint codebase, and build / server-nestjs are all FAILURE. Must be green before merge.
[blocker] apps/server-nestjs/src/modules/healthz/healthz.controller.ts:23-30 — urlKey uses raw env names ('KEYCLOAK_DOMAIN', 'GITLAB_URL', …), but configService.get(namespace) returns the transformed config (keys domain, url, …) from each factory's .transform(). So config?.[probe.urlKey] is always undefined → every optional probe is silently skipped. Fix: urlKey: 'url' (and 'domain' for keycloak), or expose a uniform probeUrl from each config factory.
[warning] apps/server-nestjs/src/main.module.ts:1,27 — getDotenvPaths is imported but never used (likely the lint failure). ConfigModule.forRoot() is given no envFilePath; it loads .env by default but the previous .env.integ conditional is gone. Either wire envFilePath: getDotenvPaths() or confirm integ loading is intentionally handled elsewhere.
[warning] main.module.ts:27 ConditionalModule.registerWhen(..., 'USE_KEYCLOAK') — treats an unset env var as false, but every config schema defaults USE_X to 'true' and no .env-example defines USE_*. Verify a default deploy still registers Keycloak/auth; prefer registerWhen(m, env => env.USE_KEYCLOAK !== 'false') to match schema defaults.
Compliance checklist
- Conventional title / module-config-isolation pattern followed
- No leaked secrets (env-examples are placeholders)
- CI green (unit-tests + lint + server-nestjs build failing)
- Healthz probes actually fire (urlKey mismatch)
- Default-deploy module registration verified
shikanime
left a comment
shikanime
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed the config → module-definition migration (142 files). Architecture is sound: zod-validated registerAs factories per module, token ownership via MODULE_OPTIONS_TOKEN, setExtras re-export, and deletion of the 179-line ConfigurationService monolith is a net win. Two blockers break runtime behaviour in prod/integ deployments and are posted as inline comments below.
BLOCKER 1 — healthz external probes never registered
configService.get('gitlab') returns the transformed object from registerAs (keys { url, internalUrl, enabled, … }), not the raw env var. So config?.['GITLAB_URL'] is always undefined, the gate is always false, and every external-service health check is silently skipped (endpoint reports green). Fix: use the transformed field name (url for gitlab/vault/nexus/registry/argocd/opencds, domain for keycloak).
BLOCKER 2 — .env.integ / .env.docker no longer loaded
ConfigModule.forRoot() with no envFilePath drops the integ/docker override chain that AGENTS.md documents as supported. utils/dotenv.utils.ts (getDotenvPaths()) reimplements the exact old chain but is never wired in — only its own spec references it. Fix: ConfigModule.forRoot({ envFilePath: getDotenvPaths() }).
WARNINGS
argocd.module.spec.tsgates ArgoCD onUSE_GITLAB/USE_VAULTinstead ofUSE_ARGOCD— the test asserts nothing about ArgoCD's actual gate.keycloak-secret-provider.service.ts:85dead?? url.host/?? url.protocolfallback after theif (!domain)early-return above.
NITS
config.utils.tscsv()is unused dead code.base.config.tsduplicatesci/isCIfrom the sameraw.CI.
Positives: zod schemas centralise env validation; ConfigurableModuleClass correctly handles .asProvider() via the isConfigProvider branch; moduleRef.get(…, { strict: false }) is the right optional-probe pattern for conditionally-loaded services.
| if (!instance) continue | ||
| const config = this.configService.get<Record<string, unknown>>(probe.namespace) | ||
| if (config?.[probe.urlKey]) checks.push(() => instance.check(probe.namespace)) | ||
| } |
There was a problem hiding this comment.
BLOCKER: config[probe.urlKey] is always undefined. configService.get('gitlab') returns the transformed object from registerAs — keys are { url, internalUrl, enabled, … }, NOT raw env vars (GITLAB_URL). So config?.['GITLAB_URL'] is always undefined and every external-service health check is silently skipped (endpoint reports green, no error).
Fix: change urlKey to the transformed field name — url for gitlab/vault/nexus/registry/argocd/opencds and domain for keycloak. config?.url is undefined when the env var is empty, so gating still works.
| @Module({ | ||
| imports: [ | ||
| ConfigModule.forRoot(), | ||
| BaseModule.forRoot(baseConfigFactory.asProvider()), |
There was a problem hiding this comment.
BLOCKER: ConfigModule.forRoot() loads only .env/.env.${NODE_ENV}. The old ConfigurationModule also loaded .env.integ (INTEGRATION=true) and .env.docker (DOCKER=true) per the AGENTS.md override chain. That chain is reimplemented in utils/dotenv.utils.ts (getDotenvPaths()) but never wired in — it's only referenced by its own spec, so integ/docker deployments lose their config.
Fix: ConfigModule.forRoot({ envFilePath: getDotenvPaths() }), or at minimum envFilePath: ['.env', '.env.integ', '.env.docker'].
| vi.stubEnv('USE_GITLAB', 'false') | ||
| const module = await Test.createTestingModule({ | ||
| imports: [ConfigModule.forRoot(), ConditionalModule.registerWhen(ArgoCDModule.forRoot(argocdConfigFactory.asProvider()), 'USE_GITLAB')], | ||
| }).compile() |
There was a problem hiding this comment.
WARNING: this asserts ArgoCD is gated on USE_GITLAB, not USE_ARGOCD. ArgoCD is imported via 'USE_ARGOCD' everywhere else (healthz.module, main wiring). Disabling an unrelated flag makes the test pass regardless of ArgoCD gating; it would stay green even if ArgoCD loaded unconditionally.
Fix: use 'USE_ARGOCD' (both occurrences) so the test actually exercises ArgoCD's gate.
| url.host = this.config.keycloakDomain ?? url.host | ||
| url.protocol = this.keycloakConfig.protocol ?? url.protocol | ||
| url.host = this.keycloakConfig.domain ?? url.host | ||
| this.logger.log(`Replacing JWKS URI domain: ${jwksUri} -> ${url.toString()}`) |
There was a problem hiding this comment.
WARNING: dead fallback. domain is already truthy here — replaceJwksUriDomainWithInternalDomain early-returns at the if (!this.keycloakConfig.domain) guard above. protocol defaults to 'https' and is always defined. The ?? url.host / ?? url.protocol branches are unreachable.
Fix: url.host = this.keycloakConfig.domain (drop the ??).
| // Comma-separated string -> array of schema-validated, trimmed non-empty parts. | ||
| // Empty/missing -> []. Strictly takes a z string schema (applied per element). | ||
| export function csv<T extends z.ZodType<string, z.ZodTypeDef, unknown>>(schema: T) { | ||
| return z |
There was a problem hiding this comment.
NIT: csv() is never imported anywhere (only flag/nonEmpty/truthySchema are used). New dead code — remove (YAGNI).
| isProd: raw.NODE_ENV === 'production', | ||
| integration: raw.INTEGRATION, | ||
| ci: raw.CI, | ||
| devSetup: raw.DEV_SETUP, |
There was a problem hiding this comment.
NIT: redundant field — both ci: raw.CI (here) and isCI: raw.CI (line 27) are mapped; the BaseConfig interface declares both. Harmless, but pick one name.
cc10fe6 to
63440ac
Compare
|
🤖 Hey ! The security scan report for the current pull request is available here. |
Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I6f501bd1633ceb14edaa9bf2a324ea026a6a6964
shikanime
left a comment
shikanime
left a comment
There was a problem hiding this comment.
Review: PR #2365 — configuration to module definition
Verdict: REQUEST CHANGES (posting as a comment-review because this is a self-authored PR and GitHub forbids the REQUEST_CHANGES review event on your own PR — the verdict still stands).
PR is otherwise well-structured and release-pipeline config needs no changes (release-please always-bump-minor, hotfix→patch; server-nestjs present in ci matrices; Helm bump job intact). Do not release until the two blockers below are resolved — both are release-breaking (silent health reports, dead env override chain).
Blockers (must fix before merge)
B1 — healthz external probes silently never register (false-healthy image). healthz.controller.ts gates each optional probe on config?.[probe.urlKey], but probe.urlKey is a raw env-var name (KEYCLOAK_DOMAIN, GITLAB_URL, …) while ConfigService.get(namespace) returns the transformed registerAs object, which only exposes url in every namespace. The gate is therefore always falsy → external probes are never registered, so the container's HEALTHCHECK reports healthy while upstreams (Keycloak, Vault, registry, …) are down.
- Fix: drop
urlKeyfromHealthProbe, gate onconfig?.url. (Already prepared in worktreewt/t_ca303e1c.)
B2 — .env.docker / .env.integ no longer loaded (documented override chain dead). main.module.ts uses ConfigModule.forRoot() with no envFilePath, leaving the existing getDotenvPaths() helper unwired. The .env < .env.docker < .env.integ chain from AGENTS.md is silently broken.
- Fix:
ConfigModule.forRoot({ isGlobal: true, envFilePath: getDotenvPaths() }). TheisGlobalis also required —HealthzControllerinjectsConfigServicebutHealthzModulenever importsConfigModule, so the token isn't injectable app-wide. (Already prepared in worktreewt/t_ca303e1c.)
Warnings (should fix)
W1 — argocd.module.spec.ts gates on the wrong flags. The two tests stub USE_GITLAB=false / USE_VAULT=false but assert ArgoCD omission, while the module is actually registerWhen(…, 'USE_ARGOCD'). The tests pass for the wrong reason and would pass even if USE_ARGOCD were ignored.
W2 — dead ?? fallback in keycloak-secret-provider.service.ts. url.protocol = this.keycloakConfig.protocol ?? url.protocol and url.host = this.keycloakConfig.domain ?? url.host — both protocol and domain are non-optional in the config schema, so the fallback is unreachable noise.
Notes
- Nit re-check:
csv()is now used (project.controller.ts:30) andisCIis a single field (no duplicate) — both prior nits are already resolved at this head, nothing to flag. - Follow-up (non-blocking):
healthz.controller.ts:1importsConfigServiceasimport type, which erases the DI token fromdesign:paramtypes. The app boots becauseisGlobalsupplies the token, but a value import is more robust. Consider changing to a value import. - Precedence detail (moot today): on a rare DOCKER+INTEGRATION combo,
getDotenvPaths()returns[.env.integ, .env.docker]so.env.dockerwins, while AGENTS.md documents.env.integas strongest. Align if that combo is ever supported.
A ready-to-commit fix for B1+B2 lives in worktree branch wt/t_ca303e1c (typecheck + lint + dotenv unit tests green). Happy to rebase it onto the current PR head if you'd like the diff applied directly.
| | undefined | ||
| if (!instance) continue | ||
| const config = this.configService.get<Record<string, unknown>>(probe.namespace) | ||
| if (config?.[probe.urlKey]) checks.push(() => instance.check(probe.namespace)) |
There was a problem hiding this comment.
B1 (blocker): config?.[probe.urlKey] is always falsy — urlKey is a raw env-var name, but ConfigService.get(namespace) returns the transformed registerAs object which only exposes url. External probes never register, so the image reports healthy while upstreams are down. Gate on config?.url instead and drop urlKey from HealthProbe.
| interface HealthProbe { | ||
| readonly service: new (...args: any[]) => { check: (name: string) => Promise<unknown> } | ||
| readonly namespace: string | ||
| readonly urlKey: string |
There was a problem hiding this comment.
B1 (blocker): remove readonly urlKey: string from the interface — it carries a raw env-var name that nothing in the transformed config exposes. The namespace's url field is the real signal.
|
|
||
| @Module({ | ||
| imports: [ | ||
| ConfigModule.forRoot(), |
There was a problem hiding this comment.
B2 (blocker): ConfigModule.forRoot() leaves getDotenvPaths() unwired, so .env.docker / .env.integ are never loaded and the documented override chain is dead. Use ConfigModule.forRoot({ isGlobal: true, envFilePath: getDotenvPaths() }). isGlobal is also required so ConfigService is injectable into HealthzModule (which never imports ConfigModule).
| it('omits ArgoCDService when USE_GITLAB=false', async () => { | ||
| vi.stubEnv('USE_GITLAB', 'false') | ||
| const module = await Test.createTestingModule({ | ||
| imports: [ConfigModule.forRoot(), ConditionalModule.registerWhen(ArgoCDModule.forRoot(argocdConfigFactory.asProvider()), 'USE_GITLAB')], |
There was a problem hiding this comment.
W1 (warning): this test stubs USE_GITLAB=false but asserts ArgoCD omission, while the module is gated on 'USE_ARGOCD'. Stub USE_ARGOCD instead, or the test passes for the wrong reason.
| const url = new URL(jwksUri) | ||
| url.protocol = this.config.keycloakProtocol ?? url.protocol | ||
| url.host = this.config.keycloakDomain ?? url.host | ||
| url.protocol = this.keycloakConfig.protocol ?? url.protocol |
There was a problem hiding this comment.
W2 (warning): dead ?? fallback — protocol and domain are non-optional in the config schema, so url.protocol/url.host never actually fall through. Drop the ?? url.* to avoid unreachable noise.
…attern Consolidate the server-nestjs configuration into the module-definition pattern introduced across feature modules. Each plugin now declares its own config dependencies through a ConfigurableFeatureModule helper and a dedicated module-definition file, replacing the previous global registerAs object wiring performed in main.module. This keeps config co-located with the module that consumes it and removes the shared config lookups that read transformed env keys at runtime. Signed-off-by: Shikanime Deva <william.phetsinorath@shikanime.studio> Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: I1836ab5c8d5eba847539fc158634cfbf6a6a6964
63440ac to
f76cb2c
Compare
| @@ -0,0 +1,74 @@ | |||
| import { afterEach, describe, expect, it, vi } from 'vitest' | |||
| import { ArgoCDHealthService } from '../argocd/argocd-health.service' | |||
| @@ -0,0 +1,74 @@ | |||
| import { afterEach, describe, expect, it, vi } from 'vitest' | |||
| import { ArgoCDHealthService } from '../argocd/argocd-health.service' | |||
| import { DatabaseHealthService } from '../infrastructure/database/database-health.service' | |||
| import { afterEach, describe, expect, it, vi } from 'vitest' | ||
| import { ArgoCDHealthService } from '../argocd/argocd-health.service' | ||
| import { DatabaseHealthService } from '../infrastructure/database/database-health.service' | ||
| import { KeycloakHealthService } from '../keycloak/keycloak-health.service' |
|
🤖 Hey ! The security scan report for the current pull request is available here. |
|
1 participant
Quel est le comportement actuel ?
La configuration des modules serveur-nestjs était dispersée : imports utilitaires non suffixés (
crypto.ts,http-error.ts,iterable.ts) et pas de point d'entrée de définition de module dédié pour certains plugins (sonarqube, vault).Quel est le nouveau comportement ?
*.module-definition.tspoursonarqubeetvault(définition centralisée du module)..utils.ts(crypto→crypto.utils,http-error→http.utils,iterable→iterable.utils) pour aligner avec les conventions du repo.dotenv.utils.ts(+ spec) et ajustement des imports dans les tests e2e.Commits :
refactor: configuration to nestjs systemRefactor config to module definitionCette PR introduit-elle un breaking change ?
Non (refactor interne, pas de changement d'API publique).
Autres informations
Testé via les specs unitaires colocalisées et les e2e existants.