Remove obsolete permissions module #49664
Conversation
dstandish
force-pushed
the
remove-permissions-module
branch
from
7130b62 to
35d8420
Compare
ashb
left a comment
ashb
left a comment
There was a problem hiding this comment.
How does per-DAG permissions work today in Airflow 3 I wonder....?
There was a problem hiding this comment.
I have a feeling that removing this file might break some old versions of providers -- we must have added this for a reason right?
There was a problem hiding this comment.
I assumed so. I asked vincent about it and he said go ahead and chop. So I was just gonna see what happens with tests. But, we don't really need to remove this it was just something I noticed yesterday.
There was a problem hiding this comment.
I guess @vincbeck could comment on your questions. It's def not my area of expertise.
There was a problem hiding this comment.
To be honest I dont remember if this is used in older version of providers :/ When I look at #47553, this PR adds RESOURCE_DAG_VERSION in compat provider but it is not used anywhere. So it might be something we added for nothing. Ideally FAB permissions should only be used in FAB. We can always remove it and if some providers are using them they can use older version of compat provider?
There was a problem hiding this comment.
Do these permissions have anything to do with airflow? Or is it 100% fab? I.e. is there any reason for them to be in anything other than fab provider? Or used by anything other than fab provider?
There was a problem hiding this comment.
These permissions are 100% FAB. Airflow delegates entirely authn and authz to auth managers so there is no such notion of "airflow permission". The only reason why a provider would use these permissions is plugin. If a provider provides an Airflow 2 plugin in the codebase, it might use FAB permissions. Example: A provider exposes a view/page, and use FAB permissions to configure who has access to this page
There was a problem hiding this comment.
As @ashb commented -> we should not remove stuff from compat until it is needed. That's generally the purpose of common.compat.
Generally (see slack discussion) https://apache-airflow.slack.com/archives/C06K9Q5G2UA/p1745534416301429 - we should not remove anything from common.compat - until we get them to "end-of-life". Common.compat contains code that supports old provider versions (like FAB) that we cannot upgrade together with other providers - and as long as we want to still release providers that are Airflow 2 compatible we need to keep the compatibility code as it was and common.compat should not have stuff removed (basically common.compat should be 1.+. That also include test code in common.compat (currently still removed). If we need to remove stuff from common-core that will make the test break - we should move it to common.compat.
The common.compat is shared - which means that for Airflow 2 the same version of compat is used by new providers released now and old providers (like fab1.*) that are needed to be installed in Airfllow 2 - which basically means that until we support new providers to work on Airflow 2, common compat should keep version 1 and not remove code.
This will change when we stop supporting providers for Airflow 2 (12 months from now). Then we will be able to bump common.compat to 2.* and remove all the stuff that was only needed for Airflow 2 -only providers.
| ) | ||
| else: | ||
| from airflow.providers.common.compat.security.permissions import ( | ||
| from airflow.providers.fab.www.security.permissions import ( |
There was a problem hiding this comment.
The if branch and else branch are then the same (for this import), we can move it out from if TYPE_CHECKING: and add it to the list of line 70
| if TYPE_CHECKING: | ||
| from airflow.providers.fab.www.security.permissions import ( | ||
| RESOURCE_ASSET, | ||
| RESOURCE_ASSET_ALIAS, | ||
| ) | ||
| else: | ||
| from airflow.providers.common.compat.security.permissions import ( | ||
| from airflow.providers.fab.www.security.permissions import ( | ||
| RESOURCE_ASSET, | ||
| RESOURCE_ASSET_ALIAS, | ||
| ) |
| ) | ||
| else: | ||
| from airflow.providers.common.compat.security.permissions import ( | ||
| from airflow.providers.fab.www.security.permissions import ( |
There was a problem hiding this comment.
Redundant, no need to do if TYPE_CHECKING
dstandish
force-pushed
the
remove-permissions-module
branch
from
bcdc9b0 to
c7b2fae
Compare
dstandish
force-pushed
the
remove-permissions-module
branch
from
6808997 to
a61bbc8
Compare
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
github-actions
bot
added
the
stale
4 participants
Apparently we don't need this anymore? I guess it was moved to the fab provider, but we just missed deleting it.