Repository navigation
refactor(cli): add shared Metrics HTTP command foundation - #122
rtiman-spotify wants to merge 2 commits into
Conversation
Co-Authored-By: Xirp Agent (gpt-6.1-sol) <xirp-agent@users.noreply.ghe.spotify.net>
Co-Authored-By: Xirp Agent (gpt-6.1-sol) <xirp-agent@users.noreply.ghe.spotify.net>
There was a problem hiding this comment.
Hi! I looked through the code and left a few comments from the technical side but, unfortunately, I don't have access to the board you linked to this PR so I can't judge the functionality itself since I lack the context about the feature.
Would be nice to get a reference with the info, purpose, and the reasons behind the feature. Thanks!
P.S. I think we should avoid linking internal resources in public repos 🙃
| import { prepareAuthTokens } from '@spotify-confidence/testing/auth'; | ||
| import { dataCommand } from '@commands/index.js'; | ||
|
|
||
| function run(args: string[]) { |
There was a problem hiding this comment.
We have a createRunner util in testing for this, please use that function instead
There was a problem hiding this comment.
Better to test a command as a whole (integration-style) than a handler in isolation
| server.use( | ||
| http.post('https://onboarding.confidence.dev/v1/agentTelemetryKey:acquire', () => | ||
| HttpResponse.json({ clientSecret: 'test' }), | ||
| ), | ||
| http.post('https://events.eu.confidence.dev/v1/events:publish', async ({ request }) => { | ||
| const body = (await request.json()) as { events: { payload: { step: string } }[] }; | ||
| steps.push(body.events[0].payload.step); | ||
| return HttpResponse.json({}); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Should be extracted in testing package for better collocation with other MSW handlers
| import { extractFlags, formatJson, message, print, resolveFormat } from '@output/index.js'; | ||
|
|
||
| /** Preserve the full response in JSON; render generic resources as key/value rows otherwise. */ | ||
| export function printData(data: unknown, argv: Record<string, unknown>): void { |
There was a problem hiding this comment.
For displaying the results, we have print in @output. Please, re-use that util; if it lacks functionality, extend it instead of creating a duplicate
| export function isRecord(value: unknown): value is Record<string, unknown> { | ||
| return typeof value === 'object' && value !== null && !Array.isArray(value); | ||
| } |
There was a problem hiding this comment.
Looks more like a part of core?
| export function resourcePath(name: string, collection: string): string { | ||
| if (!/^[A-Za-z][A-Za-z0-9]*$/u.test(collection)) { | ||
| throw new Error('Invalid resource collection.'); | ||
| } | ||
| const prefix = `${collection}/`; | ||
| if (!name.startsWith(prefix) || !/^[A-Za-z0-9][A-Za-z0-9_-]*$/u.test(name.slice(prefix.length))) { | ||
| throw new Error(`Resource name must be ${collection}/<id> with a single valid ID.`); | ||
| } | ||
| return `/v1/${name}`; | ||
| } |
There was a problem hiding this comment.
Doesn't seem to be related to @network; please, extract into features/data
| const [group, action] = argv._; | ||
| if ( | ||
| argv['dry-run'] === true && | ||
| (group === 'data' || (group === 'warehouse' && (action === 'list' || action === 'get'))) |
There was a problem hiding this comment.
Why this exception in particular? If necessary, please, extract into a util and name appropriately to better explain the purpose and context
|
|
||
| // Read the body under the same deadline as fetch. Transport/body failures | ||
| // belong to the outer catch; only JSON decoding failures are handled here. | ||
| const text = await response.text(); |
There was a problem hiding this comment.
Consuming the response as text and then parsing it via JSON.parse feels awkward. Instead, please, add an option in the config for how to parse the response
| import { metricsRequest } from '@network/metrics.js'; | ||
| import { isRecord, requireRecord, requireString, resourcePath } from '@network/metrics-types.js'; | ||
|
|
||
| afterEach(() => vi.unstubAllEnvs()); |
There was a problem hiding this comment.
Please, prefer explicit resource management over afterEach/beforeEach utils
| import type { Argv } from 'yargs'; | ||
| import { noop } from '@spotify-confidence/shared-kernel'; | ||
|
|
||
| /** Feature PRs register their subcommands in this group. */ |
There was a problem hiding this comment.
This comment doesn't add any missing context or explain quirks related to the code; please, clean up such comments throughout the PR to make the code less noisy
2 participants
Summary
Shared foundation for separate CLI resource features replacing the proposed Data MCP adapter. Adds direct regional Metrics HTTP helpers, validated resource paths, consistent output/errors, bounded cancellation, and an extensible data command group. Existing commands remain unchanged; no onboarding, discovery, run storage, or MCP registration.
Feature PRs carry this exact foundation commit until it is merged by a human. Please review this PR first. Do not merge dependent feature PRs before their prerequisites.
Verification
Node 24; pnpm build and pnpm qa passed. Existing E2E suite passed: 46 tests across 15 files. Built root/data help passed. HTTP tests use MSW; no live API compatibility claim.
Checklist
Related: https://spotify.atlassian.net/browse/SLMD-1921. No Jira edits or merge performed.