fix: vault secret path - #2412
Conversation
16472bd to
a561157
Compare
shikanime
left a comment
There was a problem hiding this comment.
Review — PR #2412 (reviewer)
Verdict: REQUEST CHANGES ⛔ (cannot formally block my own PR, but these must be resolved before merge)
Blocker 1 — Vault secret-path restructure with no migration / dual-read
This PR moves Vault paths with no backward-compatibility read and no data migration:
tech/NEXUS→forge/{slug}/NEXUS(nexus.service.tsgetProjectVaultPath(..., 'NEXUS'))- global
GITLAB→forge/{slug}/GITLAB(newgenerateGitlabTriggerTokenPath,writeMirrorTriggerToken(projectSlug, ...))
On an environment with existing secrets, the new code reads a path that has no secret yet, and the old secret at the previous path is never read or cleaned up. Needs either a one-off migration (write new path from old, or reconcile in a cron) or documented manual steps before merge.
Worse: the Fastify plugins/gitlab still reads/writes the global GITLAB path (plugins/gitlab/src/class.ts:486, functions.ts:53,134). The two backends already disagree on Vault path conventions, so any cutover reasoning is more urgent, not less — please confirm the migration plan covers the legacy plugin path too.
Blocker 2 — Dead dependency
PR body / tracking says it depends on #2403/#2405/#2406. #2406 is CLOSED without merging. Either re-open/merge it or drop the reference. #2403 and #2405 are still open.
Warnings
validateProjectTokenrelies onerror.cause?.response.status === 401. The unit tests cover 401/502/active/revoked and pass; a one-line comment citing the expectedGitbeakerRequestError.causeshape would help the next reader.mirrorTokenRotationThresholdDaysremoved; rotation is now driven by live token validity (1-year expiry). Confirm ops expects no age-based early rotation.
Nits
- e2e renames
Controller→Serviceand the newhandleDeletee2e improve coverage — good.
Build/lint/59 gitlab+nexus+vault specs: green. The Build application (client) job is green here (no client code touched).
424d68a to
032f455
Compare
Review: PR #2412 vs #2400 / #2401Three-axis review (Security → Correctness → Standards) on PR head Verdict: APPROVE — 1 non-blocking warningBlockersNone. WarningW1 — Nit
What is correct
Tests run: 16 passed on PR head. |
032f455 to
e95d42c
Compare
StephaneTrebel
left a comment
There was a problem hiding this comment.
Donc on est d'accord que ni le titre de la MR, ni le message de commit ne sont exacts ? Car là on corrige vault et nexus.
Non pas que ça me dérange, mais je préfère qu'on soit explicites 😉
StephaneTrebel
left a comment
There was a problem hiding this comment.
Me suis mélangé les crayons avec les reviewers 😅
StephaneTrebel
left a comment
There was a problem hiding this comment.
je remets mon Request Changes
StephaneTrebel
left a comment
There was a problem hiding this comment.
je pense que GH est bien buggé là 😅
5adeeca to
dbed1ce
Compare
Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: Ibf9a5522c0ccedcb1a49c087008db9a56a6a6964
Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr> Change-Id: Ibf9a5522c0ccedcb1a49c087008db9a56a6a6964
dbed1ce to
7970476
Compare
|
StephaneTrebel
left a comment
There was a problem hiding this comment.
Toujours un commit qui mélange Vault et Nexus, je dis ça… 😛

0 New Issues
0 Fixed Issues
0 Accepted Issues
Issues liées
Issues numéro: #2400 #2401
Quel est le comportement actuel ?
Quel est le nouveau comportement ?
Cette PR introduit-elle un breaking change ?
Autres informations