noron12234 commented on PR #71386:
URL: https://github.com/apache/airflow/pull/71386#issuecomment-5263243525
You were right to push back, and the answer is the first of your two
options: the check does not do what it is supposed to do. My PR described a
real bug but understated the problem, and my "87 files" number was wrong. Here
is the double-check.
**The check currently validates nothing at all.**
`provider_files_pattern` globs
`<ROOT_DIR>/airflow/providers/**/provider.yaml`:
```python
ROOT_DIR = pathlib.Path(__file__).resolve().parents[2]
provider_files_pattern = pathlib.Path(ROOT_DIR, "airflow",
"providers").rglob("provider.yaml")
```
There is no `airflow/providers/` directory in the repo any more — the 105
`provider.yaml` files live at `providers/<name>/provider.yaml`. So on current
`main`:
```
ROOT_DIR = /.../airflow
provider.yaml found = 0
get_providers_modules = 0 modules
```
With `provider_modules == []`, both branches of `modules_to_validate` come
out empty — the explicit-argv one because nothing can match, and the `else` one
because it *is* `provider_modules`. `iter_check_template_fields` is never
called and the script always exits 0.
That is why no fixes were required after my change: nothing was being
checked before it and nothing is being checked after it.
**What the `rstrip` bug is worth once the glob is repaired.**
Pointing the glob at `providers/` instead:
```
modules found = 337
of those whose last segment ends in 'p' or 'y' = 28
```
Those 28 are exactly the ones `pyfile.rstrip(".py")` truncates out of
existence:
```
airflow.providers.apache.livy.operators.livy
rstrip -> airflow.providers.apache.livy.operators.liv in
provider_modules = False
removesuffix -> airflow.providers.apache.livy.operators.livy in
provider_modules = True
airflow.providers.google.cloud.sensors.bigquery
rstrip -> airflow.providers.google.cloud.sensors.bigquer in
provider_modules = False
removesuffix -> airflow.providers.google.cloud.sensors.bigquery in
provider_modules = True
airflow.providers.ftp.operators.ftp
airflow.providers.ftp.sensors.ftp
airflow.providers.alibaba.cloud.sensors.oss_key
...
```
So the correct framing is: 28 of 337 registered operator/sensor modules, not
87 files. I got that number by counting every provider `.py` file rather than
the modules actually registered in `provider.yaml`. Sorry for the noise.
**What I cannot show you**
Whether repairing both then surfaces genuine `template_fields` violations.
`iter_check_template_fields` imports each provider module, and this script
lives under `scripts/in_container/` for a reason — outside the CI image all 337
imports fail on my machine. That part needs a `breeze` run.
**How would you like to proceed?**
1. I extend this PR to fix the glob as well, so the check actually runs, and
we find out from CI whether the 337 modules are clean.
2. You would rather the check be removed — it has been inert for long enough
that nobody noticed, which is evidence in itself. Happy to close this and open
that instead.
I did not want to widen the scope unilaterally, especially given your second
option. Your call.
--
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]
