moomindani commented on PR #69998: URL: https://github.com/apache/airflow/pull/69998#issuecomment-5343140938
Thanks for checking this independently — your reading matches mine on both counts, including the detail that the Airflow 3 branches in `get_link` are unreachable rather than merely wrong. On who does the work: that is @PrakshiGoyal10's call as the author, not mine. I have no objection either way, and offering a patch on this PR rather than opening a competing one is the right way round. One technical note before anyone writes code, because there may be a cleaner fix than persisting the URLs. Plugin links are not unconditionally replaced by `XComOperatorLink`: `_deserialize_operator` walks `plugins_manager.get_operator_extra_links()` and matches each link's `operators` against the serialized `task_type` / `_task_module`, and plugin links then take precedence over the xcom-key form (`serialization/serialized_objects.py:1088-1101` and the `op_predefined_extra_links.update(op_extra_links_from_plugin)` line). All three links in this plugin currently declare no `operators`, which is why they get replaced. Declaring them on `WorkflowJobRepairAllFailedLink` and `WorkflowJobRepairSingleTaskLink` would keep the real objects on the API server and let `_build_repair_url` run at request time — no URL duplicated into XCom, no `base_url` frozen at execute time, and the branches this PR already added would actually be exercised. Persisting into XCom the way the job-run link does also works and has the virtue of matching existing precedent, so I am not arguing hard for one over the other. Worth deciding deliberately rather than by default, and either way the `_get_launch_task_id_v3` fix and the `SerializedDagModel.get_dag` round-trip test are needed — that test is the part that stops this class of break from returning silently, so I would treat it as non-optional. On scope: the two blockers plus that test are what gate merge from my side. The `start_time` fragility is a genuine crash risk in the same flow and is welcome in the same pass; the unmapped-key warning on legacy repair-all and the `base_url` form-action / redirect notes are lower priority and can equally be follow-ups if this PR is already long in the tooth. --- Drafted-by: Claude Code (Opus 5); reviewed by @moomindani before posting -- 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]
