kaxil commented on code in PR #70499:
URL: https://github.com/apache/airflow/pull/70499#discussion_r3668288744
##########
registry/src/_data/types.json:
##########
@@ -58,5 +58,10 @@
"id": "toolset",
"label": "Toolsets",
"icon": "U"
+ },
+ {
+ "id": "retry_policy",
Review Comment:
Every other id in this file has a matching `--color-<id>` in
`registry/src/css/tokens.css` and a `.tab-icon.<id>` rule in `main.css`, and
neither exists for `retry_policy`, so the badge falls through to the bare
`.tab-icon` rule with no background. It's visible in your own screenshot: the
O/H/@/U pills are colored, R is plain text. #70122 added both lines when it
introduced `toolset`.
##########
airflow-core/src/airflow/provider.yaml.schema.json:
##########
@@ -397,6 +397,30 @@
]
}
},
+ "retry-policies": {
+ "type": "array",
+ "items": {
+ "type": "object",
+ "properties": {
+ "integration-name": {
+ "type": "string",
+ "description": "Integration name. It must have a
matching item in the 'integration' section of any provider."
Review Comment:
Nothing enforces this for `retry-policies`. `check_invalid_integration` in
`scripts/in_container/run_provider_yaml_files_check.py` iterates a hardcoded
`["sensors", "operators", "hooks", "triggers", "bundles", "toolsets"]`, and
#70122 added `toolsets` to it. That same list gates the `python-modules`
existence check via `check_if_objects_exist_and_belong_to_package`, so a typo
in the module path silently yields an empty category instead of a CI failure.
One caveat if you add it: the module-completeness half won't port over
verbatim, since `parse_module_data` globs `**/{resource_type}/*.py` and
common.ai's policy lives in `policies/retry.py`, not `retry-policies/`.
##########
dev/registry/registry_tools/types.py:
##########
@@ -121,6 +128,7 @@
("bundle", "airflow.dag_processing.bundles.base.BaseDagBundle"),
("operator", "airflow.sdk.bases.operator.BaseOperator"),
("toolset", "pydantic_ai.toolsets.abstract.AbstractToolset"),
+ ("retry_policy", "airflow.sdk.definitions.retry_policy.RetryPolicy"),
Review Comment:
The provider.yaml validator keeps its own copy of this mapping.
`check_all_provider_classes_are_registered` has a `base_class_resource_map`
with no `RetryPolicy` entry, and it builds `registered_modules` from a tuple
that omits `retry-policies`. So a provider can ship a `RetryPolicy` subclass,
forget to declare it, and CI stays green, which is the discoverability gap this
PR is closing. #70122 added `(AbstractToolset, "toolsets")` plus the tuple
entry. `LLMRetryPolicy` is the only subclass in-tree right now, so adding both
shouldn't turn anything red.
--
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]
