Repository navigation
Go SDK: vendor the schemas it generates from - #74047
Conversation
FrankYang0529
left a comment
There was a problem hiding this comment.
LGTM. Thanks for the PR.
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the follow-up, we need to make sure the sync hook is a manual step only.
Otherwise, we will encounter the same CI failure in the future.
go-sdk reached across the monorepo for both schemas it generates from — airflow-core's Dag serialization schema and task-sdk's supervisor wire-schema snapshot — so a generated file was only explainable from a directory the published Go module does not contain, and every change to a Python schema was a change that had to carry regenerated Go with it, from whoever happened to be editing the Python. Both are now vendored under go-sdk/schema/, the way ts-sdk and java-sdk already vendor theirs, and the generators read the copies. That splits one question into two: the new sync-go-sdk-schemas hook copies an original over its copy when they differ and fails, since copying is mechanical and needs neither Go nor the network; and check-go-sdk-generated-drift regenerates from the copies, since what to do about a new schema construct is a decision. Regenerating from the copies produces byte-identical output, and the two Go tests that read a schema now read the copy, so they no longer skip or break outside the monorepo.
Review feedback: running the sync hook on every commit still fails the PR of whoever changed a Python schema, which is the coupling vendoring is meant to remove, so it becomes manual-only the way sync-java-sdk-dag-schema is. The trade-off is that nothing now announces a stale copy on its own. The three format_report tests assert the whole report rather than fragments of it, and a both-refreshed case covers how two refreshes are separated.
df14c7e to
782d109
Compare
|
|
I think as long as we record this down in the SDK wise release management process that we should make sure the SDK side catchup with the minor (or major) release. Somehow similar to the i18n translation freeze before the release. Nightly job might be too often, I think we could have a bi-weekly job as follow-up. Both are valid but not that urgent at this moment if we compare them to the feature gap. |
Why
go-sdkreached across the monorepo for both schemas it generates from —airflow-core's Dagserialization schema and
task-sdk's supervisor wire-schema snapshot. Two things follow from that:go generateand two Go tests only work inside the monorepo.messages_test.gocarried a skip forexactly this.
whoever happened to be editing the Python — Go toolchain, network fetch of go-jsonschema and all.
That is what
check-go-sdk-generated-drift(Go SDK: bring the coordinator-protocol models back to the supervisor schema #73963) asked of them.ts-sdkandjava-sdkalready vendor their copies (ts-sdk/schema/dag-schema.json,java-sdk/sdk/schema/) and split the work into a cheap sync plus a generation check.go-sdkwasthe only SDK still reaching out. Raised by @jason810496 in review of #73963.
What
Both schemas are vendored under
go-sdk/schema/and the generators read the copies. Regeneratingfrom them produces byte-identical output — no
.gen.gochanges in this PR.That splits one question into two hooks:
sync-go-sdk-schemas(new,scripts/ci/prek/sync_go_sdk_schemas.py) — is the copy equal toits original? Copying is mechanical, so it copies for you and fails, the way
sync-ts-sdk-dag-schemadoes. Pure file comparison: no Go toolchain, no network. Its failuremessage names what to regenerate, and for the supervisor schema it also names
SupervisorSchemaVersion, which has to move whenapi_versiondoes.check-go-sdk-generated-drift— is the committed Go what the copy generates? It now watchesthe copies rather than the Python originals, since what to do about a new schema construct (a
generator rule, an authoring exclusion) is a decision rather than a copy.
normalize_test.goandmessages_test.goread the vendored copies, so the latter's skip is gone:the version check now runs in a standalone checkout too.
The copies are named
dag-schema.jsonandsupervisor-schema.json, not*.schema.json, so thelint-json-schemahook (files: .*\.schema\.json$) does not rewrite them and break byte-equalitywith their source — the same reason
ts-sdknamed its copy that way.Left alone deliberately:
go-sdk/airflow/spec_test.gostill readstrigger_rule.pyandweight_rule.pyfromairflow-core. That is a tripwire against Python enums, not generated code,and vendoring a
.pyintogo-sdkis a separate decision.related: #73963, #73954
Was generative AI tooling used to co-author this PR?