HYBIM-853 Bug fix for openapi upgrade#72
Conversation
…i-python-client 0.29.0 - Rewrite _LOOP_RE in patch_http_validation_error.py to match 0.29.0 from_dict shape (_detail pop before type annotation, if _detail is not UNSET wrapper, loop without or []) - Update _loop_replacement to emit list[...] | Unset style (no Union) - Regenerate all 1136 files under src/splunk_ao/resources/ with 0.29.0 (Google-style docstrings, from __future__ import annotations, response variable naming, union reordering) - Confirmed patch re-applied: isinstance(_detail, list) guard present in http_validation_error.py - Confirmed generator warnings unchanged vs 0.26.x baseline (spec-level issues only, out of scope) Co-Authored-By: Claude <noreply@anthropic.com>
fercor-cisco
left a comment
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.
Verdict: request_changes — Patch script and validation-error fix are correct and tested; but the manual metric_key_alias edit to the generated openapi.yaml is not durable and will be lost on the next spec import.
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
scripts/patch_http_validation_error.py:54-64: The patch script itself has no unit test. Given its regex is tightly coupled to the generator's exact output (and already broke silently on the 0.26.x -> 0.29.0 bump), a small pytest that feeds a representative 0.29.0from_dictsnippet throughpatch()and asserts the isinstance-guarded result — plus an idempotency case — would catch future generator drift in CI rather than at regen time.
| metric_key_alias: | ||
| anyOf: | ||
| - type: string | ||
| - type: 'null' | ||
| title: Metric Key Alias | ||
| description: Alternate metric key for this column. When scorer UUIDs are used | ||
| as column IDs (e.g. "metrics/{uuid}"), this holds the legacy snake_case | ||
| metric name (e.g. "correctness") for display and dual-key query fallback. | ||
| None for non-metric columns. |
There was a problem hiding this comment.
🟠 major (design): This metric_key_alias field is added by hand-editing openapi.yaml, but openapi.yaml is a generated artifact: scripts/import-openapi-yaml.sh regenerates it from the live server's openapi.json. Because the server spec does not include metric_key_alias on the ColumnInfo schema, the next run of import-openapi-yaml.sh will overwrite this file and silently drop the field again — reintroducing the exact 5 test failures this PR is fixing. This is the same "lost patch on regeneration" failure mode the PR is otherwise hardening against for the HTTPValidationError patch.
The durable location for local spec fixups is the yq patch block in scripts/import-openapi-yaml.sh (see the existing precedent there: StepType.enum += ["control"], with the comment "server omits this value from the spec but returns it" and "add the openapi.yaml patches here"). Please add the metric_key_alias injection to that yq expression (e.g. set .components.schemas.ColumnInfo.properties.metric_key_alias) so it survives a spec re-import. Note LogRecordsColumnInfo and the metric_* schemas already carry the field in the server spec, so only ColumnInfo needs the manual patch.
🤖 Generated by the Astra agent
| title: Roll Up Method | ||
| description: Default roll-up aggregation method for this metric (e.g., 'sum', | ||
| 'average'). | ||
| metric_key_alias: |
There was a problem hiding this comment.
Manually adding code to this file will not work. These edits will be lost on the next automated update.
If changes are necessary in Galileo's api definition that needs to be done on https://github.com/rungalileo/api
2 participants
Summary
without Union)
naming, and union reordering
0.29.0's stricter generation dropped it, breaking 5 tests
What was broken before this PR
Commit e121f41 (HYBIM-777) bumped openapi-python-client to 0.29.0 in pyproject.toml/poetry.lock but never regenerated the client. As a result:
Verification
Test plan