Copilot commented on code in PR #74415: URL: https://github.com/apache/airflow/pull/74415#discussion_r4209968263
########## ts-sdk/scripts/ci/prek/sync_ts_sdk_schemas.py: ########## @@ -0,0 +1,127 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Refresh the TypeScript SDK's vendored copies of the two schemas Python owns. + +``ts-sdk`` generates its Dag field interfaces from ``airflow-core``'s Dag serialization +schema and its supervisor wire types from the snapshot the Python Task SDK owns. It +vendors both under ``ts-sdk/schema/`` so a published npm package can be built without the +monorepo, the way ``go-sdk`` and ``java-sdk`` already vendor theirs. + +Vendoring splits the one question ("is the TypeScript behind Python?") into two, and both +now run on every commit that touches either side: + +* this hook — is the copy equal to the source? Copying is mechanical, so it copies for + you and fails, triggered by either the Python source or the vendored copy changing. + A Python-only schema change used to leave the supervisor copy unrun and nothing else + caught it either, so ``src/generated/supervisor.ts`` shipped stale. Failing here, on + the PR that caused it, is the fix. +* ``check-ts-sdk-dag-schema`` / ``check-ts-sdk-supervisor-schema`` — are the generated + files what the copy generates? They regenerate from whatever this hook leaves the copy + holding, which is why they stay separate hooks: a new schema construct may need a + generator change, which is a decision, not a copy. + +Run it from the repo root, through prek: + + prek run sync-ts-sdk-schemas + +or directly: + + ./ts-sdk/scripts/ci/prek/sync_ts_sdk_schemas.py + +Exits 0 when both copies already matched their source, 1 when it refreshed one. +""" + +from __future__ import annotations + +import pathlib +import shutil +import sys +from typing import NamedTuple + +REPO_ROOT = pathlib.Path(__file__).resolve().parents[4] + + +class VendoredSchema(NamedTuple): + """One schema Python owns and the copy ``pnpm run generate:*`` reads instead.""" + + source: pathlib.Path + vendored: pathlib.Path + # generated is what the copy feeds, and regenerate the recipe that rewrites it. + generated: str + regenerate: str + + +VENDORED_SCHEMAS = ( + VendoredSchema( + source=pathlib.Path("airflow-core/src/airflow/serialization/schema.json"), + vendored=pathlib.Path("ts-sdk/schema/dag-schema.json"), + generated="ts-sdk/src/generated/dag-schema-fields.ts", + regenerate="pnpm run generate:dag-schema", + ), + VendoredSchema( + source=pathlib.Path("task-sdk/src/airflow/sdk/execution_time/schema/schema.json"), + vendored=pathlib.Path("ts-sdk/schema/supervisor-schema.json"), + generated="ts-sdk/src/generated/supervisor.ts", + regenerate="pnpm run generate:supervisor", + ), +) + + +def refresh(schema: VendoredSchema, repo_root: pathlib.Path = REPO_ROOT) -> bool: + """Copy ``source`` over ``vendored`` when the two differ. Returns whether it copied.""" Review Comment: The new two-schema refresh and reporting behavior has no automated tests. The equivalent Go hook is covered in `scripts/tests/ci/prek/test_sync_go_sdk_schemas.py`, including in-sync, stale, missing-copy/source, and multi-refresh cases; add corresponding tests so path or control-flow regressions fail CI. ########## .pre-commit-config.yaml: ########## @@ -327,16 +327,20 @@ repos: additional_dependencies: ['PyYAML>=6.0', 'rich>=13.6.0'] pass_filenames: false require_serial: true - - id: sync-ts-sdk-dag-schema - name: Sync TypeScript SDK Dag serialization schema with airflow-core - description: "Copy airflow-core's serialization schema when TS SDK's vendored dag-schema.json drifts" - entry: ./ts-sdk/scripts/ci/prek/sync_dag_schema.py + - id: sync-ts-sdk-schemas + name: Sync TypeScript SDK vendored schemas with the distributions that own them + description: "Copy airflow-core's Dag schema and the Task SDK's supervisor schema when TS SDK's vendored copies drift" + entry: ./ts-sdk/scripts/ci/prek/sync_ts_sdk_schemas.py language: python pass_filenames: false + # Triggered by the Python sources, not just the vendored copies, so a schema change + # (e.g. a new supervisor comms message) refreshes the vendored copy and regenerates the + # types in the same commit, the way sync-go-sdk-schemas already does for the Go SDK. files: > (?x) ^airflow-core/src/airflow/serialization/schema\.json$| - ^ts-sdk/schema/dag-schema\.json$ + ^task-sdk/src/airflow/sdk/execution_time/schema/schema\.json$| + ^ts-sdk/schema/.*\.json$ Review Comment: This hook does not run when its own implementation changes, unlike the adjacent `sync-go-sdk-schemas` hook, which includes its script at `.pre-commit-config.yaml:379`. Include the new script path so changes to its source/path/report logic exercise the hook itself rather than waiting for a later schema edit. ########## .pre-commit-config.yaml: ########## @@ -327,16 +327,20 @@ repos: additional_dependencies: ['PyYAML>=6.0', 'rich>=13.6.0'] pass_filenames: false require_serial: true - - id: sync-ts-sdk-dag-schema - name: Sync TypeScript SDK Dag serialization schema with airflow-core - description: "Copy airflow-core's serialization schema when TS SDK's vendored dag-schema.json drifts" - entry: ./ts-sdk/scripts/ci/prek/sync_dag_schema.py + - id: sync-ts-sdk-schemas Review Comment: The hook rename leaves `ts-sdk/scripts/generate-dag-schema.mjs:25` referring to the removed `sync-ts-sdk-dag-schema` ID. Update that reference to `sync-ts-sdk-schemas` so both generator instructions remain runnable. ########## .pre-commit-config.yaml: ########## @@ -327,16 +327,20 @@ repos: additional_dependencies: ['PyYAML>=6.0', 'rich>=13.6.0'] pass_filenames: false require_serial: true - - id: sync-ts-sdk-dag-schema - name: Sync TypeScript SDK Dag serialization schema with airflow-core - description: "Copy airflow-core's serialization schema when TS SDK's vendored dag-schema.json drifts" - entry: ./ts-sdk/scripts/ci/prek/sync_dag_schema.py + - id: sync-ts-sdk-schemas + name: Sync TypeScript SDK vendored schemas with the distributions that own them + description: "Copy airflow-core's Dag schema and the Task SDK's supervisor schema when TS SDK's vendored copies drift" + entry: ./ts-sdk/scripts/ci/prek/sync_ts_sdk_schemas.py language: python pass_filenames: false + # Triggered by the Python sources, not just the vendored copies, so a schema change + # (e.g. a new supervisor comms message) refreshes the vendored copy and regenerates the + # types in the same commit, the way sync-go-sdk-schemas already does for the Go SDK. Review Comment: The new same-commit regeneration policy contradicts the selective-check rationale in `dev/breeze/src/airflow_breeze/utils/selective_checks.py:1946-1949` and `dev/breeze/doc/ci/04_selective_checks.md:551-554`, which still says supervisor schema changes deliberately defer TS regeneration to a follow-up PR. Update both explanations so CI policy documentation matches this hook. ########## ts-sdk/scripts/generate-supervisor.mjs: ########## @@ -20,8 +20,9 @@ // Codegen for the Airflow supervisor wire schema. // -// Reads the canonical supervisor schema from Airflow's Task SDK and emits -// `src/generated/supervisor.ts`. +// Reads the vendored supervisor schema (`schema/supervisor-schema.json`, kept in +// sync with Airflow's Task SDK by the `sync-ts-sdk-supervisor-schema` prek hook) +// and emits `src/generated/supervisor.ts`. Review Comment: This comment points contributors to `sync-ts-sdk-supervisor-schema`, but no hook with that ID exists; the new combined hook is `sync-ts-sdk-schemas`. As written, the documented command fails. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
