feat: add query command for hydrated recipe value extraction#445
feat: add query command for hydrated recipe value extraction#445
Conversation
Implements ADR-004. New `aicr query` CLI command and `/v1/query` API endpoint that resolves a recipe from criteria, hydrates all component values (base + overlay + inline overrides merged), and extracts a specific value using a dot-path selector. - CLI: same criteria flags as recipe, plus required --selector - API: GET/POST /v1/query with criteria + selector parameter - Selector: dot-delimited paths, yq-style leading dot supported - Output: scalars as plain text, complex values as YAML/JSON - Error UX: typo hints with available keys at divergence point
- Add missing sidebar entry for gke-tcpxo-networking doc - Exclude node_modules from grype scan (esbuild binary compiled with go1.23.12 was triggering false positive stdlib CVEs)
c50f4e1 to
bd1a08b
Compare
Coverage Report ✅
Coverage BadgeMerging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
yuanchen8911
left a comment
There was a problem hiding this comment.
Review: feat: add query command for hydrated recipe value extraction
Core logic (HydrateResult, Select) is clean and well-tested. ADR-004 compliance is faithful overall. A few issues to address before merge:
1. POST /v1/query bypasses criteria validation
The GET path parses criteria through ParseCriteriaFromRequest, but the POST path unmarshals the request body directly into QueryRequest.Criteria via json.Unmarshal/yaml.Unmarshal. Any validation performed by the parser is skipped. The downstream AllowLists check catches some cases, but invalid field values that the parser would reject can slip through.
pkg/recipe/handler_query.go:83-95
2. --output flag is exposed but silently ignored
queryCmdFlags() inherits all recipeCmdFlags() including --output. However, the query action never writes to a file — it always prints to stdout. A user passing --output result.yaml gets no error and no file written.
Either filter --output from the inherited flags, or wire it up to write to the specified file.
pkg/cli/query.go:29-36
3. --format json produces invalid output for scalar values
writeQueryResult prints string/bool/number scalars via fmt.Println(v) regardless of the output format. With --format json, a string scalar like 570.86.16 is printed as bare text instead of "570.86.16" — not valid JSON. Lists and maps are handled correctly.
pkg/cli/query.go:182-192
4. /v1/query endpoint missing from OpenAPI spec
The route is registered in pkg/api/server.go and the ADR calls out API parity, but api/aicr/v1/server.yaml is not updated. This means generated clients and API documentation won't include the new endpoint.
5. Code duplication: buildRecipeFromCmd
buildRecipeFromCmd in pkg/cli/query.go:120-178 is a near-verbatim copy of the recipe command action in pkg/cli/recipe.go:152-224. Consider extracting a shared helper so recipe resolution logic only lives in one place.
Minor
.grype.yamlnode_modules exclusion andmetadata_test.goconstant extractions are unrelated to the feature — consider splitting into a separate PR.pkg/cli/query.gohas 0% test coverage (70 statements). Thepkg/recipetests cover the core logic well, but CLI output formatting (writeQueryResult,writeComplexValue) is untested.
|
Addressed all items:
|
- Add Criteria.Validate() for POST body validation parity with GET path - Remove --output flag from query command (always prints to stdout) - Fix --format json for scalar values to produce valid JSON - Add /v1/query endpoint to OpenAPI spec with QueryRequest schema - Deduplicate recipe resolution by reusing buildRecipeFromCmd in recipe.go - Add CLI tests for writeQueryResult and flag filtering
yuanchen8911
left a comment
There was a problem hiding this comment.
All five items from the previous review are addressed:
- POST criteria validation —
Criteria.Validate()added with parser-equivalent checks, called in POST path before proceeding. Test covers invalid criteria returning 400. --outputflag — Filtered out ofqueryCmdFlags(). Test verifies exclusion.--format jsonscalars — All JSON output now goes throughjson.MarshalIndent, producing valid JSON for all types. Test verifies quoted strings.- OpenAPI spec — Full GET/POST
/v1/querydefinitions withQueryRequestschema, examples, and response codes. - Code duplication —
recipe.gorefactored to use sharedbuildRecipeFromCmd. Constraint warning logging correctly stays in recipe action only.
LGTM.
|
Non-blocking follow-up: If these fields are intentionally excluded as internal implementation details (file references, health check plumbing), consider documenting that in the ADR or |
The omissions are intentional and appropriate. The excluded fields fall into two categories:
|
- Add missing /v1/query endpoint documentation (exists in OpenAPI spec since #445) - Add /v1/query to feature comparison table and routes example - Add missing gke-nccl-tcpxo to bundler table - Fix content type application/yaml to application/x-yaml in curl example
- Add missing /v1/query endpoint documentation (exists in OpenAPI spec since #445) - Add /v1/query to feature comparison table and routes example - Add missing gke-nccl-tcpxo to bundler table - Fix content type application/yaml to application/x-yaml in curl example
Summary
Implements ADR-004 — a new
aicr querycommand and/v1/queryAPI endpoint that resolves a recipe from criteria and extracts a specific fully hydrated value using a dot-path selector.This addresses the gap where AICR embeds curated configuration metadata into the binary but users have no read-only way to inspect it without running a full
aicr bundleto disk.What's new
pkg/cli/query.go): same criteria flags asrecipe, plus required--selectorwith dot-path syntax (consistent with Helm--setandyq)pkg/recipe/query.go):HydrateResult()merges all base/overlay/inline values per component;Select()walks dot-path with available-keys hints on typospkg/recipe/handler_query.go):GET/POST /v1/querywith same criteria + selector parameter.components.gpu-operator.chartandcomponents.gpu-operator.chartboth workAdditional fixes
gke-tcpxo-networkingdocgoconstlint issues inmetadata_test.gonode_modulesfrom grype scan (esbuild binary compiled with go1.23.12 was triggering false positive stdlib CVEs)Test plan
make qualifypasses (test, lint, e2e, scan)query.go(12),handler_query.go(12)Select100%,HydrateResult97.5%,HandleQuery80%,parseQueryRequestFromBody85.7%make server+ curl