You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jason810496 (via GitHub)" <gi...@apache.org> on 2026/04/07 02:40:36 UTC
[PR] Follow-up fixes for double-encode trigger PR [airflow]
jason810496 opened a new pull request, #64823:
URL: https://github.com/apache/airflow/pull/64823
## Why
Address unresolved review comments from #64626
## What
- `trigger.py`: Add `# to avoid circular import: Callback` to clarify the purpose of lazy import
- `test_encoders.py`: Replace module-level pytest.importorskip with a try/except block so FileDeleteTrigger (non-Kafka) tests still run when the Kafka provider is not installed.
- `test_collection.py`: Move Trigger, encode_trigger, and BaseEventTrigger imports from inline test bodies to module scope.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up fixes for double-encode trigger PR [airflow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #64823:
URL: https://github.com/apache/airflow/pull/64823#discussion_r3066486138
##########
airflow-core/tests/unit/serialization/test_encoders.py:
##########
@@ -35,23 +57,7 @@
FileDeleteTrigger(filepath="/tmp/test.txt", poke_interval=5.0),
id="primitive_kwargs_only",
),
- pytest.param(AwaitMessageTrigger(topics=()), id="empty_tuple"),
- pytest.param(
- AwaitMessageTrigger(topics=("fizz_buzz",), poll_timeout=1.0, commit_offset=True),
- id="single_topic_tuple",
- ),
- pytest.param(
- AwaitMessageTrigger(
- topics=["t1", "t2"],
- apply_function="my.module.func",
- apply_function_args=["a", "b"],
- apply_function_kwargs={"key": "value"},
- kafka_config_id="my_kafka",
- poll_interval=2,
- poll_timeout=3,
- ),
- id="all_non_primitive_kwargs",
- ),
+ *_kafka_params,
]
Review Comment:
When the Kafka provider isn't installed, `_kafka_params` becomes empty, so all tests still run but they only exercise the primitive-only `FileDeleteTrigger` path. That means the non-primitive (e.g. tuple) wrapping/unwrapping behavior that motivated this suite isn't validated in the default/core-only test environment. Consider adding at least one always-available trigger (e.g. a `providers.standard` trigger instantiated with a tuple kwarg, or a small test-only trigger stub) to `_TRIGGER_PARAMS` so the non-primitive serialization/idempotency logic is covered even without the Kafka provider.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up fixes for double-encode trigger PR [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #64823:
URL: https://github.com/apache/airflow/pull/64823#issuecomment-4286310381
### Backport successfully created: v3-2-test
Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.
In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>✅</td>
<td>v3-2-test</td>
<td><a href="https://github.com/apache/airflow/pull/65584"><img src="https://img.shields.io/badge/PR-65584-blue" alt="PR Link"></a></td>
</tr>
</table>
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up fixes for double-encode trigger PR [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 merged PR #64823:
URL: https://github.com/apache/airflow/pull/64823
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up fixes for double-encode trigger PR [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on code in PR #64823:
URL: https://github.com/apache/airflow/pull/64823#discussion_r3108596084
##########
airflow-core/tests/unit/serialization/test_encoders.py:
##########
@@ -25,8 +25,30 @@
from airflow.serialization.enums import DagAttributeTypes as DAT, Encoding
from airflow.triggers.base import BaseEventTrigger
-pytest.importorskip("airflow.providers.apache.kafka")
-from airflow.providers.apache.kafka.triggers.await_message import AwaitMessageTrigger
+try:
Review Comment:
Good point, but this is a real edge case that we caught in Kafka `AwaitMessageTrigger`. So I refactor as `_CallableKwargsTrigger`, a unit test only class that follow the Kafka `AwaitMessageTrigger` pattern instead of directly import the provider in core tests.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up fixes for double-encode trigger PR [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #64823:
URL: https://github.com/apache/airflow/pull/64823#discussion_r3042717771
##########
airflow-core/tests/unit/serialization/test_encoders.py:
##########
@@ -25,8 +25,30 @@
from airflow.serialization.enums import DagAttributeTypes as DAT, Encoding
from airflow.triggers.base import BaseEventTrigger
-pytest.importorskip("airflow.providers.apache.kafka")
-from airflow.providers.apache.kafka.triggers.await_message import AwaitMessageTrigger
+try:
Review Comment:
should we move it to provider test instead?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up fixes for double-encode trigger PR [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on code in PR #64823:
URL: https://github.com/apache/airflow/pull/64823#discussion_r3108596084
##########
airflow-core/tests/unit/serialization/test_encoders.py:
##########
@@ -25,8 +25,30 @@
from airflow.serialization.enums import DagAttributeTypes as DAT, Encoding
from airflow.triggers.base import BaseEventTrigger
-pytest.importorskip("airflow.providers.apache.kafka")
-from airflow.providers.apache.kafka.triggers.await_message import AwaitMessageTrigger
+try:
Review Comment:
Good point, but this is a real edge case that we caught in Kafka `AwaitMessageTrigger`. So I refactor as `_CallableKwargsTrigger`, a unit test only class that follow the Kafka `AwaitMessageTrigger` error to ensure the coverage.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Follow-up fixes for double-encode trigger PR [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #64823:
URL: https://github.com/apache/airflow/pull/64823#issuecomment-4286307341
Hi maintainer, this PR was merged without a milestone set.
We've automatically set the milestone to **[Airflow 3.2.2](https://github.com/apache/airflow/milestone/138)** based on: backport label targeting v3-2-test
If this milestone is not correct, please update it to the appropriate milestone.
> This comment was generated by [Milestone Tag Assistant](https://github.com/apache/airflow/blob/main/.github/workflows/milestone-tag-assistant.yml).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org