Sandbox::https://github.com/apache/airflow/pull/16077

jhtimmins · 2021-05-26T02:56:04Z

This is a re-implementation of one piece of #15398, which uses the "resource" and "action" naming scheme.

The original PR has proved difficult to get reviewed and through CI, as it touches a lot of different files that change frequently. This PR introduces wrapper methods around default FAB methods, this time using the updated naming scheme.

The methods are not in use yet; the next PR will update method calls throughout Airflow to use the new methods instead of the default FAB methods.

jedcunningham

Overall LGTM. Minor nit, might be nice to having typing on these.

jedcunningham · 2021-05-26T14:41:42Z

Why is this super() vs self?

And should we change parameter to action_name? for consistency ?

Generally, I like to add only as much explanation as is necessary based on context. In this case it seems reasonable to infer that name refers to the name of the action being retrieved.

potiuk · 2021-05-26T14:47:02Z

I think "action_name" and "resource_name" should be here as well.

potiuk · 2021-05-26T14:48:24Z

Just double-checking - resource here is different than "resource_name" (looks like - from the type).

Yeah, resource should apply to the actual object

potiuk

Few consistency nits.

Would you also care to add Type annotations for parameters? That would make mypy verify types of the parameters in most cases - especially when there was action vs. action_name or resource vs. resource_name.

kaxil · 2021-05-26T21:35:23Z

Sphinx needs one blank line between description and params

Suggested change

Returns a resource record by name, if it exists.

:param name: Name of resource

Returns a resource record by name, if it exists.

:param name: Name of resource

kaxil · 2021-05-26T21:35:46Z

Suggested change

Gets an existing action record.

:param name: name

Gets an existing action record.

:param name: name

kaxil · 2021-05-26T21:36:01Z

Suggested change

Gets a permission made with the given action->resource pair, if the permission already exists.

:param action_name: Name of action

Gets a permission made with the given action->resource pair, if the permission already exists.

:param action_name: Name of action

kaxil · 2021-05-26T21:36:16Z

Suggested change

Creates a permission linking an action and resource.

:param action_name: Name of existing action

Creates a permission linking an action and resource.

:param action_name: Name of existing action

kaxil · 2021-05-26T21:37:18Z

Suggested change

Add an existing permission pair to a role.

:param role: The role about to get a new permission.

Add an existing permission pair to a role.

:param role: The role about to get a new permission.

kaxil · 2021-05-26T21:37:25Z

Suggested change

Remove a permission pair from a role.

:param role: User role containing permissions.

Remove a permission pair from a role.

:param role: User role containing permissions.

kaxil · 2021-05-26T21:37:34Z

Suggested change

Deletes a permission action.

:param name: Name of action to delete (e.g. can_read).

Deletes a permission action.

:param name: Name of action to delete (e.g. can_read).

kaxil · 2021-05-26T21:37:45Z

Suggested change

Retrieve permission pairs associated with a specific resource object.

:param resource: Object representing a single resource.

Retrieve permission pairs associated with a specific resource object.

:param resource: Object representing a single resource.

kaxil · 2021-05-26T21:37:59Z

Suggested change

then re-syncs them.

:return: None

then re-syncs them.

:return: None

jhtimmins · 2021-05-27T19:05:34Z

@potiuk can you take another look? I've added typing and either updated or explained the naming choices.

github-actions · 2021-05-27T20:20:15Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

potiuk

LGTM. Thanks for the explanations. Makes sense.

jhtimmins · 2021-05-29T21:35:48Z

Thanks @potiuk. Is the failing MSSQL test related to that other PR about improving MSSQL compatibility?

potiuk · 2021-05-30T05:34:56Z

Yep. We already have a fix for that. We looked for the problem for last few days and it turned out it is caused by ..... Tmpfs backing our docker engine for self-hosted runners (they are optimized for speed). Fix is coming in #16159

boring-cyborg Bot added the area:webserver Webserver related Issues label May 26, 2021

jhtimmins requested review from ashb, jedcunningham and kaxil May 26, 2021 02:56

jhtimmins force-pushed the add-fab-wrapper-functions branch from cb60630 to 6345285 Compare May 26, 2021 03:04

jedcunningham reviewed May 26, 2021

View reviewed changes

potiuk reviewed May 26, 2021

View reviewed changes

potiuk requested changes May 26, 2021

View reviewed changes

jhtimmins force-pushed the add-fab-wrapper-functions branch from 6b815c9 to 129ebf1 Compare May 26, 2021 20:57

jhtimmins requested review from jedcunningham and potiuk May 26, 2021 20:59

jedcunningham approved these changes May 26, 2021

View reviewed changes

kaxil reviewed May 26, 2021

View reviewed changes

jhtimmins force-pushed the add-fab-wrapper-functions branch from 129ebf1 to c05946a Compare May 27, 2021 01:21

jhtimmins requested a review from kaxil May 27, 2021 01:25

kaxil approved these changes May 27, 2021

View reviewed changes

jhtimmins requested review from XD-DENG, mik-laj, turbaszek and vikramkoka as code owners May 27, 2021 18:52

jhtimmins added 4 commits May 27, 2021 11:55

Add updated-name wrappers for built-in FAB methods.

d2389ce

Update typing.

b24de1c

Add spacing.

2c2e892

Add updated-name wrappers for built-in FAB methods.

1768355

jhtimmins force-pushed the add-fab-wrapper-functions branch from 08e46ad to 1768355 Compare May 27, 2021 19:02

potiuk approved these changes May 27, 2021

View reviewed changes

github-actions Bot added the okay to merge It's ok to merge this PR as it does not require more tests label May 27, 2021

potiuk approved these changes May 29, 2021

View reviewed changes

potiuk merged commit a27e930 into apache:master May 29, 2021

jhtimmins mentioned this pull request Jun 1, 2021

call resource based fab methods. #16190

Merged

kaxil deleted the add-fab-wrapper-functions branch December 31, 2025 02:20

-        Returns a resource record by name, if it exists.
-        :param name: Name of resource
+        Returns a resource record by name, if it exists.
+        :param name: Name of resource

-        Gets an existing action record.
-        :param name: name
+        Gets an existing action record.
+        :param name: name

-        Gets a permission made with the given action->resource pair, if the permission already exists.
-        :param action_name: Name of action
+        Gets a permission made with the given action->resource pair, if the permission already exists.
+        :param action_name: Name of action

-        Creates a permission linking an action and resource.
-        :param action_name: Name of existing action
+        Creates a permission linking an action and resource.
+        :param action_name: Name of existing action

-        Add an existing permission pair to a role.
-        :param role: The role about to get a new permission.
+        Add an existing permission pair to a role.
+        :param role: The role about to get a new permission.

-        Remove a permission pair from a role.
-        :param role: User role containing permissions.
+        Remove a permission pair from a role.
+        :param role: User role containing permissions.

-        Deletes a permission action.
-        :param name: Name of action to delete (e.g. can_read).
+        Deletes a permission action.
+        :param name: Name of action to delete (e.g. can_read).

-        Retrieve permission pairs associated with a specific resource object.
-        :param resource: Object representing a single resource.
+        Retrieve permission pairs associated with a specific resource object.
+        :param resource: Object representing a single resource.

-        then re-syncs them.
-        :return: None
+        then re-syncs them.
+        :return: None

Conversation

jhtimmins commented May 26, 2021

Uh oh!

jedcunningham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhtimmins commented May 27, 2021

Uh oh!

github-actions Bot commented May 27, 2021

Uh oh!

potiuk left a comment

Choose a reason for hiding this comment

Uh oh!

jhtimmins commented May 29, 2021

Uh oh!

potiuk commented May 30, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants