Skip to content

Prepare SaveMoney for future updates#1775

Merged
mamu0 merged 8 commits into
mainfrom
refactor-savemoney-infrastructure
May 21, 2026
Merged

Prepare SaveMoney for future updates#1775
mamu0 merged 8 commits into
mainfrom
refactor-savemoney-infrastructure

Conversation

@mamu0
Copy link
Copy Markdown
Contributor

@mamu0 mamu0 commented May 20, 2026

Impact-free refactoring to prepare SaveMoney for future implementations:

  • Unified Finding model + adapter
  • Interface wrapping existing analyzers
  • Limiter
  • In-memory cache
  • Rewritten orchestrator

Resolves: CES-2077

@mamu0 mamu0 requested a review from a team as a code owner May 20, 2026 13:02
Copilot AI review requested due to automatic review settings May 20, 2026 13:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the @pagopa/dx-savemoney package to prepare for future extensions by introducing a unified “Finding” domain model, a plugin-style analyzer registry, and new runtime helpers (limiter + in-run metrics caching), while keeping the current report output shape (AzureDetailedResourceReport) unchanged.

Changes:

  • Introduces a unified Finding model and an adapter to bridge legacy AnalysisResult into Finding[].
  • Adds a plugin architecture for Azure analyzers (registry + types) and rewrites the orchestrator to use it.
  • Adds a small concurrency limiter and an in-memory cache for Azure Monitor metric calls (scoped to a single run).

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
packages/savemoney/src/index.ts Re-exports new Phase 0 public APIs (Finding model, analyzer plugin layer, limiter).
packages/savemoney/src/finding.ts Adds unified Finding types and an adapter from legacy AnalysisResult.reason.
packages/savemoney/src/concurrency.ts Introduces a lightweight async concurrency limiter helper.
packages/savemoney/src/azure/utils.ts Adds run-scoped in-memory caching for getMetric() plus reset/test hooks.
packages/savemoney/src/azure/types.ts Extends AzureConfig with an optional concurrency setting.
packages/savemoney/src/azure/analyzers/types.ts Defines analyzer plugin interfaces (Analyzer, AnalyzerContext, AzureClients).
packages/savemoney/src/azure/analyzers/registry.ts Wraps existing per-resource analyzers into the new registry.
packages/savemoney/src/azure/analyzers/index.ts Provides re-exports for the analyzer plugin layer.
packages/savemoney/src/azure/analyzer.ts Rewrites orchestration to use analyzer registry + limiter; resets metrics cache at run start.
.nx/version-plans/version-plan-1779281691024.md Declares a patch version bump for the refactor.
Comments suppressed due to low confidence (1)

packages/savemoney/src/concurrency.ts:35

  • createLimiter can temporarily allow more than max tasks to run. When a task completes, next() decrements active and resolves the next waiter; before the resumed waiter increments active, a new caller can observe active < max and start too, causing active to exceed max and breaking the concurrency guarantee. Consider a pool implementation that starts queued tasks without releasing the slot (or uses a while (active >= max) await ... loop plus slot reservation) so there is no window where capacity is double-claimed.
  return async <T>(fn: () => Promise<T>): Promise<T> => {
    if (active >= max) {
      await new Promise<void>((resolve) => queue.push(resolve));
    }
    active++;

Comment thread packages/savemoney/src/azure/analyzer.ts Outdated
Comment thread packages/savemoney/src/azure/types.ts
Comment thread packages/savemoney/src/finding.ts Outdated
Comment thread packages/savemoney/src/azure/utils.ts Outdated
Comment thread packages/savemoney/src/finding.ts Outdated
Comment thread packages/savemoney/src/concurrency.ts Outdated
Comment thread packages/savemoney/src/__tests__/concurrency.test.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

packages/savemoney/src/azure/tests/utils.test.ts:167

  • Same issue here: as unknown as Parameters<typeof getMetric>[0] hides type mismatches in the failing-client mock. Using a small MonitorClientLike interface (or a Pick<> of the real client) would let this mock type-check without assertions and prevent accidental drift from the production call shape.
function makeFailingMonitorClient(calls: number[] = []) {
  return {
    metrics: {
      list: vi.fn().mockImplementation(async () => {
        calls.push(1);
        throw new Error("network error");
      }),
    },
  } as unknown as Parameters<typeof getMetric>[0];
}

Comment thread packages/savemoney/src/azure/analyzer.ts Outdated
Comment thread packages/savemoney/src/azure/analyzer.ts Outdated
Comment thread packages/savemoney/src/azure/__tests__/utils.test.ts Outdated
Comment thread packages/savemoney/src/azure/__tests__/utils.test.ts Outdated
Comment thread packages/savemoney/src/concurrency.ts Outdated
Comment thread packages/savemoney/src/azure/utils.ts
Comment thread packages/savemoney/src/concurrency.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

Comment thread packages/savemoney/src/azure/analyzer.ts
Comment thread packages/savemoney/src/azure/analyzer.ts Outdated
Comment thread packages/savemoney/package.json Outdated
@mamu0 mamu0 merged commit 9906c13 into main May 21, 2026
7 checks passed
@mamu0 mamu0 deleted the refactor-savemoney-infrastructure branch May 21, 2026 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants