moomindani commented on PR #71782: URL: https://github.com/apache/airflow/pull/71782#issuecomment-5447747191
My approval stands for the current head (`350955dfd`). The only commit pushed after the review is docs wording plus one source comment, and it is exactly the two inline notes I had left: the docs described the skipped params more narrowly than the code does (a null passed in the trigger conf resolves to `None` too, not only a nullable `Param` left unset), and the `dict()` round-trip needed a note recording why it cannot be simplified to `params.items()`. No logic changed, so the earlier verification carries over unchanged: the six new tests fail before the fix and pass after it, and the payloads the fixed operators build were sent to a live workspace — a skipped param arrives with no `value` and falls through to the job-level default, while the pre-fix shapes are rejected by the API outright (`Job-level parameters '...' is missing default value.` for `jobs/create`, `Could not parse request object: Expected both 'key' and 'value' to be set...` for `run-now`). The one item I raised as a question rather than an objection is unchanged and does not block: for `jobs/create` the forwarded value is the parameter *definition*, so skipping a `None` param drops it from the job's schema instead of defining it with an empty default. I could not find a practical consequence. --- 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]
