Check GCSToS3Operator match_glob support after template rendering

[atikulmunna]

Rewrites the match_glob support check in GCSToS3Operator.__init__ so it reads whether the argument was passed, and narrows it to ValueError.

match_glob is a template field, so the constructor only ever sees the un-rendered Jinja expression. The old check, if not self.__is_match_glob_supported and match_glob:, was a truthiness test on that string, which asks a third question matching neither intent.

It stays in __init__. It is a provision check, not a value check: it asks whether match_glob was supplied, combined with an environment capability the constructor can already answer. Per the false positives section of #70296 those belong in the constructor and get rewritten in place rather than moved, because with render_template_as_native_obj=True a provided field can render to None, and raising at construction surfaces a static authoring mistake as a Dag import error instead of once per task instance and per retry.

Since #70505 narrowed validate-operators-init to sanction is None / is not None reads, the rewrite passes the hook and the exemption entry still goes.

This change:

• rewrites the check to match_glob is not None, which is strictly stricter than the truthiness test since it also rejects match_glob=""
• narrows AirflowException to ValueError, matching Keep S3DeleteObjectsOperator validation in __init__, narrow to ValueError #70359, which also drops AirflowException from the common.compat.sdk import as nothing else in the file used it
• removes the file's now-stale gcs_to_s3.py::1 entry from generated/known_airflow_exceptions.txt, since narrowing removed its only raise AirflowException
• adds test_match_glob_requires_recent_google_provider, parametrised over a glob and the empty string so the is not None polarity is pinned, where nothing covered this error before
• removes GCSToS3Operator from scripts/ci/prek/validate_operators_init_exemptions.txt

An earlier revision of this PR moved the check into execute() instead, mirroring #70574. That was wrong for the reasons above, and @potiuk's review corrected it; the current diff is the in-place rewrite.

Verified locally:

python scripts/ci/prek/validate_operators_init.py \
  providers/amazon/src/airflow/providers/amazon/aws/transfers/gcs_to_s3.py
python scripts/ci/prek/check_new_airflow_exception_usage.py \
  providers/amazon/src/airflow/providers/amazon/aws/transfers/gcs_to_s3.py
ruff check && ruff format --check   # on the changed files
pytest providers/amazon/tests/unit/amazon/aws/transfers/test_gcs_to_s3.py

The test suite runs in a Linux container rather than natively; _shared/observability/metrics/stats.py calls os.register_at_fork at import, which is POSIX only.

related: #70296

---

Was generative AI tooling used to co-author this PR?

• Yes (please specify the tool below)

Generated-by: Claude Code following the guidelines

---

[boring-cyborg]

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
Here are some useful points:

• Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
• In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
• Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
• Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
• Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
• Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
  Apache Airflow is a community-driven project and together we are making it better 🚀.
  In case of doubts contact the developers at:
  Mailing List: dev@airflow.apache.org
  Slack: https://s.apache.org/airflow-slack

[potiuk]

Thanks — correct fix. match_glob is in template_fields, so validating it in __init__ meant a templated value was checked before rendering, and a failure there breaks Dag parsing rather than failing the task. Moving it into execute alongside the _is_match_glob_supported branch is the right place, and the version probe itself correctly stays in __init__ since it depends on the installed provider, not on the rendered value. Dropping the entry from validate_operators_init_exemptions.txt completes it.

Test is good — asserting list.assert_not_called() proves it fails before doing any work.

One consistency point inline.

---

Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

[potiuk]

Retracting my earlier approval — my mistake, and I'm sorry for the churn.

I approved this before reading #70296, which sets out the rule for this burn-down, and by that rule this change moves the wrong kind of check.

A check that only asks whether an argument was passed … belongs in __init__ and must not be moved. Fix these by rewriting in place, not by moving. Use the is not None polarity.

The reasoning in the issue is sound and I'd missed both halves of it:

• With render_template_as_native_obj=True, a field that was provided can render to None, so the same check in execute() reports a supplied argument as missing.
• Raising in the constructor surfaces a static authoring mistake as a Dag import error, rather than once per task instance and per retry on a worker.

if not self.__is_match_glob_supported and self.match_glob: asks whether match_glob was passed (combined with an environment capability that __init__ can already answer). It's a provision check, so it should be rewritten in place rather than relocated:

# in __init__ — keep it here
if not self.__is_match_glob_supported and match_glob is not None:
    raise ValueError(
        "The 'match_glob' parameter requires 'apache-airflow-providers-google>=10.3.0'."
    )

Note the is not None polarity matters: if match_glob: is a truthiness test on the un-rendered Jinja string, which is a third question that matches neither intent.

My earlier point about narrowing AirflowException to ValueError still stands and is worth doing in the same edit — #70503 makes the same call for dms.py.

Good news on mechanics: #70505 (which narrows the hook to allow provision checks written with is not None) merged on 28 July, so the rewrite below passes validate-operators-init and you can still remove the exemption-file entry in this PR — the burn-down goal is unaffected.

There's an active follow-up, #70503, cataloguing already-merged PRs that made exactly this move so they can be put back. Fixing it here saves this PR from joining that list.

---

Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

[atikulmunna]

Thanks for the correction, and no need to apologise for the churn. You're right. I mirrored #70574 and treated it as settled precedent instead of checking it against the false positives section of #70296, which is exactly the failure mode you describe in your sweep.

Rewritten in place:

• the check stays in __init__, now match_glob is not None rather than the truthiness test
• narrowed to ValueError, which also drops AirflowException from the common.compat.sdk import since nothing else in the file used it
• the test asserts the raise at construction instead of from execute()
• exemption entry still removed, and I confirmed validate-operators-init passes with it gone, so Allow argument-provision checks of templated fields in operator __init__ #70505 does what you said

One thing that wasn't in your review: narrowing to ValueError removes this file's only raise AirflowException, which left a stale gcs_to_s3.py::1 entry in generated/known_airflow_exceptions.txt. I dropped that too, same as #70359 did. Running --cleanup also wanted to remove an unrelated mlengine.py entry, which I put back so the diff stays on topic.

Also worth correcting my own PR description: I said there I couldn't run the tests. I've since got them running in a Linux container and the file is green, 31 passed. The Windows blocker was os.register_at_fork in _shared/observability/metrics/stats.py, which is POSIX only, so it isn't something a local install can work around.

The version probe stays in __init__ untouched, since it depends on the installed provider rather than a rendered value.

[shahar1]

The rewrite lands where #70296 says it should, and it matches the #70359
precedent one-for-one. Two small things below, neither blocking — I'm leaving
this as a comment rather than an approval only because the earlier
CHANGES_REQUESTED is still the standing review state.

Things I checked rather than assumed:

• match_glob is not None is the shape the hook sanctions —
  _collect_sanctioned_uses() in scripts/ci/prek/validate_operators_init.py
  explicitly whitelists is None / is not None comparisons on template
  fields, which the old truthiness test was not. Removing the
  validate_operators_init_exemptions.txt entry is correct.
• The generated/known_airflow_exceptions.txt deletion is what the generator
  itself would produce: AllowlistManager.check() does del allowlist[rel]
  rather than tightening to ::0 when a file's count reaches zero. Keeping the
  unrelated mlengine.py entry out of the diff was the right call — that file
  no longer exists, and cleaning it up belongs in its own PR.
• Shape matches f781f8b878 (#70359, S3DeleteObjectsOperator) exactly: same
  four files, same polarity, same ValueError narrowing, no changelog entry.
  Per providers/AGENTS.md § "Changelog — never use newsfragments", nothing
  further is needed.

Smaller observations

• PR description no longer matches the diff. It still describes the first
  approach — check removed from __init__, raised from execute(),
  AirflowException deliberately kept, test named
  test_execute__match_glob_requires_recent_google_provider. All four are now
  false; the second commit reversed them. It also still states the unit tests
  couldn't be run locally, which you corrected in a comment. The review
  criteria call this out directly:

  Description doesn't match code: PR description describes something
  different from what the code actually does.

  — .github/instructions/code-review.instructions.md.
  The commit messages are accurate, so this is only the body; worth a quick
  rewrite so the next reader isn't chasing an execute() change that isn't
  there.

• providers/amazon/tests/unit/amazon/aws/transfers/test_gcs_to_s3.py:79 — the
  new test doesn't lock in the is not None polarity; see the inline comment.

---

This review was drafted by an AI-assisted tool and
confirmed by an Airflow maintainer. The findings
below are observations, not blockers; an 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 Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.

---

Drafted-by: Claude Code (Opus 5); reviewed by @shahar1 before posting

[shahar1]

The test passes match_glob=f"**/*{DELIMITER}", which is truthy, so it would still pass against the old if not ... and match_glob: condition — the only thing failing it on main is the AirflowException → ValueError swap, not the polarity change. Since is not None vs. truthiness was the substance of the review round, worth parametrising so the empty-string case is pinned:

Suggested change

	@mock.patch("airflow.providers.google.__version__", "10.2.0")
	def test_match_glob_requires_recent_google_provider(self):
	with pytest.raises(ValueError, match="match_glob"):
	GCSToS3Operator(
	task_id=TASK_ID,
	gcs_bucket=GCS_BUCKET,
	prefix=PREFIX,
	dest_aws_conn_id="aws_default",
	dest_s3_key=S3_BUCKET,
	match_glob=f"**/*{DELIMITER}",
	)
	@mock.patch("airflow.providers.google.__version__", "10.2.0")
	@pytest.mark.parametrize("match_glob", [f"**/*{DELIMITER}", ""])
	def test_match_glob_requires_recent_google_provider(self, match_glob):
	with pytest.raises(ValueError, match="match_glob"):
	GCSToS3Operator(
	task_id=TASK_ID,
	gcs_bucket=GCS_BUCKET,
	prefix=PREFIX,
	dest_aws_conn_id="aws_default",
	dest_s3_key=S3_BUCKET,
	match_glob=match_glob,
	)

AGENTS.md asks for @pytest.mark.parametrize when tests differ only in input, so this stays one test rather than two.

[atikulmunna]

Both fixed in 3c8d638 plus a description rewrite.

Test: you're right, it wasn't testing the polarity at all. The truthy glob would have passed the old condition too, so the ValueError swap was doing all the work. Parametrised over the glob and the empty string as suggested. match_glob="" is the case that separates the two conditions. Both pass, file is at 32.

Description: it was still describing the first revision, including the bit about not being able to run the tests locally. Rewritten to match the diff, with a note that the earlier execute() version was corrected in review.