Skip to content

docs: add ADR-004 for hydrated recipe query command#435

Merged
mchmarny merged 3 commits intomainfrom
feat/adr-004-query-command
Mar 19, 2026
Merged

docs: add ADR-004 for hydrated recipe query command#435
mchmarny merged 3 commits intomainfrom
feat/adr-004-query-command

Conversation

@mchmarny
Copy link
Copy Markdown
Member

@mchmarny mchmarny commented Mar 19, 2026

Summary

  • Adds ADR-004 proposing a new aicr query command that resolves a recipe from criteria (same flags as recipe) and extracts a specific hydrated value by dot-path selector
  • Addresses the gap where recipe output contains value references but no way to inspect the fully merged configuration
  • Dot-path syntax (components.gpu-operator.values.driver.version) is consistent with Helm --set and yq

Test plan

  • Review ADR structure and completeness
  • Validate proposed selector syntax covers primary use cases
  • Confirm hydrated structure matches actual RecipeResult + GetValuesForComponent output

@mchmarny mchmarny requested a review from a team as a code owner March 19, 2026 17:17
@mchmarny mchmarny self-assigned this Mar 19, 2026
Clarify that embedded metadata is inaccessible until bundle time,
and that recipe output contains references not resolved values.
Copy link
Copy Markdown
Contributor

@yuanchen8911 yuanchen8911 left a comment

Choose a reason for hiding this comment

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

Clean ADR — well-motivated, good alternatives analysis, and the dot-path syntax is a pragmatic choice.

A few observations (non-blocking):

Low:

  1. Hydration of all components on every query is acknowledged as a future concern. Worth noting that GetValuesForComponent reads from the embedded FS — if values files are large (some Helm charts have 500+ line values), hydrating 15-30 components could add noticeable latency. A --component filter that short-circuits hydration to a single component would be a natural v1.1 optimization.

  2. Error UX for typos. If a user queries components.gpu-opertor.values.driver.version (typo), they get ErrCodeNotFound. Consider including the list of valid keys at the level where the path diverges (e.g., "key 'gpu-opertor' not found; available: cert-manager, gpu-operator, ..."). This is a small UX win for a read-only introspection tool.

  3. Reference links use relative paths (../../pkg/cli/recipe.go) which won't resolve on GitHub's rendered markdown. Use paths from repo root (e.g., pkg/cli/recipe.go) or full GitHub URLs.

  4. --selector '' (empty string) returns the entire hydrated tree. Worth documenting that aicr query --selector '' is equivalent to "dump everything" and could produce large output. Consider making --selector required with no default, and offering --selector . as the explicit "all" syntax (consistent with jq).

@yuanchen8911
Copy link
Copy Markdown
Contributor

Review findings:

  1. Medium: Selector contract is internally inconsistent for full response.
  • The ADR says --selector is required and uses dot-path syntax, but the All example uses an empty selector (--selector '') without defining empty-selector behavior in the path rules table.
  • This leaves implementation/test behavior ambiguous (is root selected by '', . or not allowed).
  1. Medium: Alternative fix: rename prometheus component to kube-prometheus-stack #3 suggests a workaround that does not satisfy the stated problem.
  • The ADR correctly states aicr recipe is abstract and does not expose hydrated values.
  • But Alternative fix: rename prometheus component to kube-prometheus-stack #3 says users can pipe aicr recipe --format json | jq ...; that cannot query hydrated component values, so the rationale is misleading in this ADR context.

@mchmarny
Copy link
Copy Markdown
Member Author

Responding to the review:

On point 2 (hydration cost): All recipe data — registry, overlays, component values — is embedded in the binary and read from an in-memory embed.FS. There's no disk I/O or network call. Hydrating 15-30 components means unmarshaling a few YAML files from memory and merging maps — this completes in microseconds. It's not a future concern; it's a non-issue at any realistic scale. A --component filter would add API surface for zero measurable benefit.

On point 3 (error UX for typos): Reasonable idea but out of scope for the ADR. The implementation can improve error messages without changing the design contract.

On point 4 (relative paths): These are standard ADR reference links that work fine in local browsing and IDEs. GitHub renders them correctly from the file's directory context. No change needed.

On point 5 (--selector '' vs .): Both are supported in the implementation. --selector is already required (no default). . and '' both return the full hydrated tree — this is consistent with yq/jq where . means "root." Already documented in the ADR's examples section.

@mchmarny
Copy link
Copy Markdown
Member Author

Responding to the comment:

On point 1 (empty selector contract): The implementation supports both '' and . as "select root" — the leading dot is stripped, and an empty result means "return everything." This is consistent with yq where . is the identity operator. The path rules table covers explicit paths; the "All" example immediately above it shows the empty-selector behavior. No ambiguity in practice.

On point 2 (Alternative #3 workaround): The sentence you're referencing says "Users who need advanced filtering can pipe aicr recipe --format json | jq '...'." This is in the context of why we don't need to embed a full jq/JMESPath library — the alternative is about query expressiveness (array filters, projections, wildcards), not about hydration. Users needing advanced filtering on the hydrated output would pipe aicr query --selector . | jq '...'. The distinction is between query syntax power and value hydration — the ADR is rejecting the former, not the latter.

@mchmarny mchmarny merged commit 658a7db into main Mar 19, 2026
11 checks passed
@mchmarny mchmarny deleted the feat/adr-004-query-command branch March 19, 2026 23:25
xdu31 pushed a commit to xdu31/aicr that referenced this pull request Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants