Re: [PR] Fix RESOURCE_ASSET compatibility with Airflow 2.x in common-compat [airflow]
jedcunningham merged PR #64933: URL: https://github.com/apache/airflow/pull/64933 -- 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]
Re: [PR] Fix RESOURCE_ASSET compatibility with Airflow 2.x in common-compat [airflow]
Copilot commented on code in PR #64933: URL: https://github.com/apache/airflow/pull/64933#discussion_r3066484873 ## providers/common/compat/src/airflow/providers/common/compat/security/permissions.py: ## @@ -16,8 +16,16 @@ # under the License. from __future__ import annotations +from airflow.providers.common.compat.version_compat import AIRFLOW_V_3_0_PLUS + # Resource Constants RESOURCE_BACKFILL = "Backfills" RESOURCE_DAG_VERSION = "DAG Versions" -RESOURCE_ASSET = "Assets" RESOURCE_ASSET_ALIAS = "Asset Aliases" + +if AIRFLOW_V_3_0_PLUS: +RESOURCE_ASSET = "Assets" +else: +from airflow.security.permissions import ( +RESOURCE_DATASET as RESOURCE_ASSET, # noqa: F401 # type: ignore[attr-defined, no-redef] +) Review Comment: The conditional import of `RESOURCE_DATASET` happens after module-level assignments (`RESOURCE_BACKFILL`, etc.), which will trigger Ruff E402 (imports not at top of file) for this module. To avoid CI/lint failures, move the `if AIRFLOW_V_3_0_PLUS` block (and the import) above the resource constant assignments, or otherwise ensure the import occurs before any non-import statements (alternatively, use a literal string for the AF2 value if you want to avoid conditional importing). ## providers/common/compat/src/airflow/providers/common/compat/security/permissions.py: ## @@ -16,8 +16,16 @@ # under the License. from __future__ import annotations +from airflow.providers.common.compat.version_compat import AIRFLOW_V_3_0_PLUS + # Resource Constants RESOURCE_BACKFILL = "Backfills" RESOURCE_DAG_VERSION = "DAG Versions" -RESOURCE_ASSET = "Assets" RESOURCE_ASSET_ALIAS = "Asset Aliases" + +if AIRFLOW_V_3_0_PLUS: +RESOURCE_ASSET = "Assets" +else: +from airflow.security.permissions import ( +RESOURCE_DATASET as RESOURCE_ASSET, # noqa: F401 # type: ignore[attr-defined, no-redef] +) Review Comment: This change fixes a version-dependent permission constant, but there’s no regression test that asserts the *value* of `RESOURCE_ASSET` for the running Airflow major version (the existing import-only test won’t catch the Airflow 2.x vs 3.x mismatch). Add an assertion-based unit test that checks the expected value under Airflow 2.x vs 3.x (e.g., branch on `AIRFLOW_V_3_0_PLUS`). -- 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]
Re: [PR] Fix RESOURCE_ASSET compatibility with Airflow 2.x in common-compat [airflow]
potiuk commented on PR #64933: URL: https://github.com/apache/airflow/pull/64933#issuecomment-4217596539 There is a mypy issue for providers :( -- 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]
Re: [PR] Fix RESOURCE_ASSET compatibility with Airflow 2.x in common-compat [airflow]
jedcunningham commented on PR #64933: URL: https://github.com/apache/airflow/pull/64933#issuecomment-4215408661 An example after the upgrade (all default roles get this) (this is before this fix of course): https://github.com/user-attachments/assets/a5e84f9f-54f7-4eff-9063-ebd0c39a2834"; /> -- 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]
Re: [PR] Fix RESOURCE_ASSET compatibility with Airflow 2.x in common-compat [airflow]
jedcunningham commented on PR #64933: URL: https://github.com/apache/airflow/pull/64933#issuecomment-4215296830 I suspect it wasn't found in the compatibility tests because it caused "Assets" resources and permissions to be created alongside the "Dataset" ones that should have been used. And if you didn't have a custom role (or maybe modified the existing roles) without the dataset perms, its hidden because it "just works", even if its fundamentally wrong. -- 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]
Re: [PR] Fix RESOURCE_ASSET compatibility with Airflow 2.x in common-compat [airflow]
potiuk commented on PR #64933: URL: https://github.com/apache/airflow/pull/64933#issuecomment-4210392283 I guess that means rc2 for common.compat and the two providers that depend on it's new version from the current vote. Thanks for finding and fixing! -- 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]
Re: [PR] Fix RESOURCE_ASSET compatibility with Airflow 2.x in common-compat [airflow]
potiuk commented on PR #64933: URL: https://github.com/apache/airflow/pull/64933#issuecomment-4210403721 BTW. I **do** think that the `if AIRFLOW*` pattern with version is way bettter than "Pythonic" try/except - and this is the reason I was pushing for it @ashb (and @xBis7 as you asked) - the main reason for having this pattern is to **know** exactly when you can remove it - one of the reasons this one happened was that it was not at all clear if we can remove that try/remove. Also we will have to find out why this was not detected by compatibility tests - it should have been, but possibly this kind of issue was masked by something. To be diagnosed. -- 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]
