Fix Cloud Monitoring list operators with explicit output formats - #73434
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
shahar1
left a comment
There was a problem hiding this comment.
Thanks for the fix — it maps one-for-one onto the issue's acceptance criteria, and the
regression tests are well scoped: they fail without the operator change, and the existing
default-format tests still assert the format_=None protobuf conversion.
One edge case worth tightening, left inline. Not blocking.
CI hasn't run on this PR yet (first PR to the repo — workflow runs need maintainer
approval), so I'm leaving this as a comment rather than an approval until the checks
are green.
As noted in #73431, I'll carry this over to the cloud_monitoring rename in #73354
before that one merges.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The findings
below are observations, not blockers; an Apache Airflow
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Apache Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
0f52c5f to
aa4e3aa
Compare
|
I also went through the updated diff manually and verified the new regression tests. Thanks again, @shahar1, for catching this compatibility edge case. |
Please fix static checks |
Avoid converting dictionaries and JSON strings as protobuf objects while retaining XCom-compatible default results. Generated-by: OpenAI Codex (GPT-6)
Unrecognized formats still return protobuf objects from the hook and must remain XCom-serializable. Generated-by: OpenAI Codex (GPT-6)
aa4e3aa to
0c99789
Compare
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Explicit
format_="dict"andformat_="json"currently fail because both Cloud Monitoring list operators attempt to convert the hook's already-formatted output as protobuf objects. Preserve that output unchanged for"dict"and"json", and retain protobuf-to-dictionary conversion forNoneand unrecognized values for XCom serialization.Update both operator docstrings and add parametrized regression tests for both explicit formats. The existing default-format tests remain unchanged. Six additional regression cases cover
"Dict","protobuf", and the empty string across both operators.Closes #73431.
Rebased onto the merged rename in #73354. The fix and regression tests now target
cloud_monitoring.py; the deprecated Stackdriver aliases remain unchanged.After rebasing onto #73354, Linux Breeze verification passed all 54 tests across the Cloud Monitoring and deprecated Stackdriver operator/hook test files. Reverting the implementation makes all four explicit-format regression cases fail as expected. Ruff and
git diff --checkalso passed. The refreshed upstream CI is running.Initial implementation validation passed in Linux Breeze CI:
The unit tests and regression verification also passed in an independent run. Local Ruff formatting/linting and
git diff --checkpassed. Local Breeze testing was blocked by missing Docker on Windows, so the test and check suites ran on Linux. Verification workflows live only on separate fork branches and are not part of this PR.Was generative AI tooling used to co-author this PR?
Generated-by: OpenAI Codex (GPT-6) following the guidelines