Skip to content

feat(server-nestjs): migrate observability plugin to NestJS module - #2419

Merged
shikanime merged 3 commits into
mainfrom
wphetsinorath/push-qvxyrootzuuu
Aug 7, 2026
Merged

feat(server-nestjs): migrate observability plugin to NestJS module#2419
shikanime merged 3 commits into
mainfrom
wphetsinorath/push-qvxyrootzuuu

Conversation

@shikanime

Copy link
Copy Markdown
Member

Change-Id: I94218bb605555fbeea90572320e78e5a6a6a6964

Issues liées

Issues numéro: #2418


Quel est le comportement actuel ?

Quel est le nouveau comportement ?

Cette PR introduit-elle un breaking change ?

Autres informations

@shikanime shikanime added this to the 9.24.0 milestone Aug 5, 2026
@shikanime
shikanime requested a review from a team August 5, 2026 08:59
@shikanime shikanime self-assigned this Aug 5, 2026
@shikanime shikanime added technical debt Résoud de la dette technique tech Technical issue labels Aug 5, 2026
@shikanime
shikanime marked this pull request as draft August 5, 2026 08:59
@github-actions github-actions Bot added the built label Aug 5, 2026
@shikanime
shikanime force-pushed the wphetsinorath/push-qvxyrootzuuu branch from f6d4793 to 5fb5dbc Compare August 5, 2026 09:06
@shikanime

Copy link
Copy Markdown
Member Author

Review — PR #2419 (feat(server-nestjs): migrate observability plugin)

Verdict: REQUEST CHANGES (author == reviewer, so this is a non-approving review; self-approval is blocked by GitHub).

Reviewed against live head f6d4793 + issue #2418 acceptance criteria. 12 files, +869/-0.

Blockers

B1 — Values repo is created in the wrong GitLab group path
observability-client.service.ts:37 calls this.gitlab.getRepos(OBSERVABILITY_GROUP_NAME) ('observability'). getRepos resolves through getOrCreateProjectSubGroup(projectSlug)getOrCreateGroupByPath(projectRootDir + '/observability'), i.e. it looks for the group nested under the console project-root-dir, and lists projects there via Groups.allProjects. The real global observability group is top-level. So findOrCreateValuesRepo will either create a spurious projectRootDir/observability group and then createGroupRepo the values repo under it (…/observability/observability) instead of observability/observability.
Fix: resolve the group by root path (getOrCreateGroupByPath(OBSERVABILITY_GROUP_NAME) already does this correctly on line 33) and list its projects by id, e.g. this.client.Groups.allProjects(group.id) via offsetPaginate — add a small getGroupRepos(groupId) helper instead of reusing getRepos.

B2 — ObservabilityPluginService is never wired into PluginService
plugin/plugin.service.ts:26-34 hardcodes the plugin infos() list. observability is absent, so ObservabilityPluginService.infos() (the Grafana ServiceInfos — an explicit #2418 scope item) is dead code and Grafana links never render in the UI. Every other migrated plugin (gitlab, sonarqube, nexus, registry, vault, argocd, keycloak) is registered here.
Fix: add ['observability', () => this.observabilityPlugin?.infos(projectId)] plus @Inject(ObservabilityPluginService) @Optional() private readonly observabilityPlugin? to PluginService.

B3 — No unit tests shipped
#2418 acceptance: "Tests unitaires Vitest (datastore, utils, service) — 0 régression". The PR adds 0 *.spec.ts. The perm computation, buildObservabilityProject YAML shape, and Keycloak group reconcile are non-trivial and unchecked. Add at minimum observability.utils.spec.ts (getListPerms / buildObservabilityProject / path generators) and a service spec with mockDeep clients — matching the existing plugin.service.spec.ts / sonarqube patterns.

Warnings

W1 — Permission heuristic grants RW to every role (no read-only)
observability.utils.ts:104-110: ro = true; rw = rw || perms > 0n whenever any bit is set, so rw is always true whenever ro is true. The -RO and -RW Grafana groups therefore get identical members — the RO group is pointless and least-privilege is lost. The comment "Write permission is the highest bit" is dead/misleading (bits aren't actually inspected). Verify against the legacy plugin's real bit test (likely perms & WRITE_BIT); if legacy truly did this, keep but drop the misleading comment.

W2 — USE_OBSERVABILITY missing from .env.integ-example
#2418 lists it as a finish criterion. .env-example and .env.docker-example got it, but apps/server-nestjs/.env.integ-example did not (it has GRAFANA_URL and DSO_OBSERVABILITY_CHART_VERSION but no USE_OBSERVABILITY=false).

Nits

  • observability.utils.ts:31 interface Tenant {} is dead (never populated, always {}). Delete.
  • observability.utils.ts:105-110 dead comment describing bit logic that isn't implemented.

Good

  • Conditional activation via registerWhen(ObservabilityModule, 'USE_OBSERVABILITY') matches the established pattern.
  • Reuses GitlabClientService / KeycloakClientService (no client duplication); createGroupRepo addition is reasonable.
  • capturePluginResult + @OnEvent + @StartActiveSpan + telemetry attributes follow the migrated-module convention (gitlab/sonarqube).
  • Config via Zod observabilityConfigFactory is consistent with other modules.

Fix B1–B3 to unblock.

@shikanime
shikanime force-pushed the wphetsinorath/push-qvxyrootzuuu branch 8 times, most recently from 293ba15 to 038e49a Compare August 5, 2026 11:50
@shikanime
shikanime marked this pull request as ready for review August 5, 2026 11:51
@shikanime
shikanime force-pushed the wphetsinorath/push-qvxyrootzuuu branch 2 times, most recently from 1165957 to e1be596 Compare August 5, 2026 12:14
@shikanime shikanime added the preview Deploy preview app with Argo-cd label Aug 5, 2026
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

🤖 Hey !

A preview of the application is available at : https://console-pr-2419.dso.cpin-hp.numerique-interieur.fr

Please be patient, deployment may take a few minutes.

StephaneTrebel
StephaneTrebel previously approved these changes Aug 5, 2026

@StephaneTrebel StephaneTrebel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quelques remarques mais c'est vachement mieux qu'avant haha

Comment thread apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts Outdated
Comment thread apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts
Comment thread apps/server-nestjs/src/modules/gitlab/gitlab-client.service.ts
@shikanime
shikanime force-pushed the wphetsinorath/push-qvxyrootzuuu branch 3 times, most recently from 1490a23 to a0a1d0d Compare August 6, 2026 09:42
Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr>
Change-Id: I94218bb605555fbeea90572320e78e5a6a6a6964
@shikanime
shikanime force-pushed the wphetsinorath/push-qvxyrootzuuu branch from a0a1d0d to 32b0d8a Compare August 6, 2026 09:53
@shikanime
shikanime force-pushed the wphetsinorath/push-qvxyrootzuuu branch from 32b0d8a to b0ad55d Compare August 6, 2026 09:55
@shikanime

Copy link
Copy Markdown
Member Author

Review: #2419 — feat(server-nestjs): migrate observability plugin

Verdict: APPROVE (reviewer agent)

Re-verified against head b0ad55d960 (force-pushed since earlier pass). Observability module bridges project.upsert/project.delete into capturePluginResult('observability', …) — mirrors the per-entity bridge every other migrated plugin uses; authz + plugin-hook parity preserved. gitlab-client.service.ts now handles the Gitbeaker "has already been taken" race (idempotent reload), defaultBranch sourced from shared defaultBranchName, and getOrCreateProjectGroupPublicUrl composes the URL correctly. Specs present (observability.service.spec.ts, observability.utils.spec.ts). No blocker.

Non-blocking: confirm no e2e assertion still expects the pre-migration path (gated).

Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr>
Change-Id: I5d0036f9cfcf9c6fca0113e3331aadca6a6a6964
Signed-off-by: William Phetsinorath <william.phetsinorath-open@interieur.gouv.fr>
Change-Id: I5d0036f9cfcf9c6fca0113e3331aadca6a6a6964
@shikanime
shikanime force-pushed the wphetsinorath/push-qvxyrootzuuu branch from d9be974 to 8a45e88 Compare August 6, 2026 12:43
@cloud-pi-native-sonarqube

Copy link
Copy Markdown

@StephaneTrebel StephaneTrebel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus rien qui m'interroge, on va pouvoir intégrer ça au reste 👍

@shikanime
shikanime added this pull request to the merge queue Aug 7, 2026
Merged via the queue into main with commit 5ba3bf8 Aug 7, 2026
60 of 62 checks passed
@shikanime
shikanime deleted the wphetsinorath/push-qvxyrootzuuu branch August 7, 2026 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

built preview Deploy preview app with Argo-cd tech Technical issue technical debt Résoud de la dette technique

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants