jykae commented on PR #68074: URL: https://github.com/apache/airflow/pull/68074#issuecomment-4646180992
Thanks for the careful review @jscheffl (and Copilot)! Just pushed b9824f9 addressing all the comments. Highlights: - **`decide_action` rewritten** to use a `_REVISION_HEADS_MAP` reverse lookup + `packaging.version.Version` comparison, with a nearest-lower fallback so patch versions like `3.2.2` resolve to `3.2.0`s head (matches Airflow CLIs own behaviour). Dropped the `ScriptDirectory.walk_revisions` plumbing entirely — much less coupling. - **`@retry`** on `discover_api_server_pod` (5 attempts, exponential backoff) so an api-server rollover doesnt fail the job. - **`drainTimeoutSeconds`** chart value (default 300) plumbed in as `MIGRATE_JOB_DRAIN_TIMEOUT_SECONDS` for operators with long-running worker tasks. - **`args: []`** is now distinguished from `args: ~` (`kindIs "invalid"` check) so the empty-list override is respected. - **Tests** restructured around `patch_engine_returning` / `make_pod` / `fake_stream` / `make_workload` fixture factories, with a new `test_decide_action_resolves_patch_version_via_nearest_lower` regression test for the patch-version mapping bug. - **Newsfragment** updated to cover all four actions (added the fresh-install path), ticket references dropped throughout. - **`values.schema.json`** updated for the new `drainTimeoutSeconds` key. Follow-up: Ill open a separate PR to expose a public accessor in `airflow.utils.db` so this script can drop the `_REVISION_HEADS_MAP` underscore once 3.3 lands. Full chart test suite (115 tests across `test_basic_helm_chart` + `test_migrate_database_job*` + `test_db_migrate_script`) passes locally. -- 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]
