Vamsi-klu commented on PR #63567: URL: https://github.com/apache/airflow/pull/63567#issuecomment-4061373946
Hey @SameerMesiah97, this looks good — replacing the list-and-scan with a direct `.get()` call is a clear win, and it follows the same pattern already used elsewhere in this provider (e.g., `wasb.py` for blob existence checks, `key_vault.py` for secrets). One thing worth calling out that the PR description doesn't mention: the old `test_exists_with_existing` test was actually buggy. It asserted `False` even for the "exists" case, because `ContainerGroup.name` on the mock was `None` and never matched the lookup name. So the positive case was never really tested. Your new tests fix this — nice catch even if it was accidental. A couple of small things: - **Missing newsfragment** — since this is a provider improvement, you'd probably want one at `providers/microsoft/azure/newsfragments/63567.improvement.rst`. - **Minor:** in `test_exists_unexpected_exception`, `pytest.raises(Exception)` is pretty broad. Adding `match="Unexpected Exception"` would make it more precise, though not a blocker. Otherwise this is clean and ready to go from my perspective. -- 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]
