uranusjr commented on a change in pull request #18690:
URL: https://github.com/apache/airflow/pull/18690#discussion_r723747637
##########
File path: tests/test_utils/api_connexion_utils.py
##########
@@ -14,26 +14,43 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from contextlib import contextmanager
+
from airflow.api_connexion.exceptions import EXCEPTIONS_LINK_MAP
from airflow.www.security import EXISTING_ROLES
-def create_user(app, *, username, role_name, email=None, permissions=None):
+@contextmanager
+def create_user_scope(app, **kwargs) -> None:
+ """
+ Helper function designed to be used with pytest fixture mainly.
+ It will create a user and provide it for the fixture via YIELD (generator)
+ then will tidy up once test is complete
+ """
+ test_user = create_user(app, **kwargs)
+ if kwargs.get('no_roles', None):
+ test_user.roles = []
+ try:
+ yield test_user
+ finally:
+ delete_user(app, kwargs.get('username'))
+
+
+def create_user(app, **kwargs):
appbuilder = app.appbuilder
+ username = kwargs.get('username')
+ role_name = kwargs.get('role_name')
Review comment:
Why not make these appear directly in the argument list? Also the use of
`get` here is likely wrong; `username` and `role_name` cannot be `None` (which
is what `get` returns if the key is missing), so those arguments should be
required in the first place.
Instead of adding a `no_roles` argument in the We should change `role_name`
in the original function signature to optional, and clear the roles if
`role_name` is None.
Something like
```python
def create_user(app, *, username, role_name=None, email=None,
permissions=None):
...
if role_name is None:
# This automatically reuses the built-in role.
role = appbuilder.sm.find_role("Public")
else:
delete_role(app, role_name)
role = create_role(app, role_name, kwargs.get('permissions'))
return appbuilder.sm.add_user(...)
@contextmanager
def setup_user(app, *, username, role_name=None, email=None,
permissions=None):
user = create_user(
app,
username=username,
role_name=role_name,
email=email,
permissions=permissions,
)
if role_name is None:
user.roles = []
try:
yield user
finally:
delete_user(app, username)
```
##########
File path: tests/test_utils/api_connexion_utils.py
##########
@@ -14,26 +14,43 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from contextlib import contextmanager
+
from airflow.api_connexion.exceptions import EXCEPTIONS_LINK_MAP
from airflow.www.security import EXISTING_ROLES
-def create_user(app, *, username, role_name, email=None, permissions=None):
+@contextmanager
+def create_user_scope(app, **kwargs) -> None:
Review comment:
Same here; `kwargs` should be an explicit argument list. Also the return
type is wrong.
```suggestion
def create_user_scope(app, *, username, role_name=None, email=None,
permissions=None):
```
##########
File path: tests/test_utils/api_connexion_utils.py
##########
@@ -14,26 +14,43 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+from contextlib import contextmanager
+
from airflow.api_connexion.exceptions import EXCEPTIONS_LINK_MAP
from airflow.www.security import EXISTING_ROLES
-def create_user(app, *, username, role_name, email=None, permissions=None):
+@contextmanager
+def create_user_scope(app, **kwargs) -> None:
+ """
+ Helper function designed to be used with pytest fixture mainly.
+ It will create a user and provide it for the fixture via YIELD (generator)
+ then will tidy up once test is complete
+ """
+ test_user = create_user(app, **kwargs)
+ if kwargs.get('no_roles', None):
+ test_user.roles = []
+ try:
+ yield test_user
+ finally:
+ delete_user(app, kwargs.get('username'))
+
+
+def create_user(app, **kwargs):
appbuilder = app.appbuilder
+ username = kwargs.get('username')
+ role_name = kwargs.get('role_name')
Review comment:
Also, should we just turn `create_user` into a context manager and
always use it? Or are there instances where we _don't_ want the user to be
deleted? That feels wrong to me.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
