You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/09/28 21:17:17 UTC
[GitHub] [airflow] khalidmammadov opened a new pull request #18590: Fixing bug when roles list is empty
khalidmammadov opened a new pull request #18590:
URL: https://github.com/apache/airflow/pull/18590
This PR fixes a bug when "roles" dictionary key is checked for none existence and provides false positive when field does exist and is an empty array.
e.g. given below dictionary
```
d=dict(role=[])
if d.get('role'): # will yield False!!!
do_something
```
but
```
if 'role' in d: # is True
do_something
```
will provide true
I also renamed **users_list** variable names to **users_list_** as there is a function with the same name **users_list** in the module. This can help to avoid possible future issues.
This PR also fixes below flaky test:
tests/cli/commands/test_user_command.py::TestCliUsers::test_find_user_exceptions
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r732237155
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -174,12 +189,19 @@ def users_import(args):
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list: List[Dict[str, Any]]):
appbuilder = cached_app().appbuilder
users_created = []
users_updated = []
for user in users_list:
+
+ try:
+ # Validate structure
+ UserSchema().load(user)
+ except ValidationError as e:
+ raise SystemExit(f"Error: Can't load user \"{user}\". \nDetails:{e}")
Review comment:
`user` is a dict here so quoting it is probably not going to be particularly legible. We should probably find another way. I wonder if it'd be better to validate `users_list` in its entirety instead; Marshmallow provides pretty good context on list items out of the box (something like *Item 1 in list is invalid*).
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r730571040
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -174,12 +175,23 @@ def users_import(args):
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list_: List[Dict[str, Any]]):
Review comment:
Why the additional underscore?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-930474988
You are right, it's not even allowed to create a user without a role. In fact code will fail with KeyNotFound error as there is a check for valid role names above of that code and that accesses "roles" field optimistically.
So, I think best thing would be to move this "required" field check block further up and just after "for" so it fails fast when they are not provided.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r780840273
##########
File path: tests/cli/commands/test_user_command.py
##########
@@ -45,18 +48,9 @@ def _set_attrs(self, app, dagbag, parser):
self.dagbag = dagbag
self.parser = parser
self.appbuilder = self.app.appbuilder
- self.clear_roles_and_roles()
+ delete_users(app)
Review comment:
Is this change going to have any side affects on other tests? The two functions do pretty different things.
`clear_roles_and_roles` (maybe this should have been names `clear_users_and_roles`?) only deletes the test users and roles used in these tests
`delete_users` deletes all users (and leaves the roles intact).
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r732690604
##########
File path: tests/cli/commands/test_user_command.py
##########
@@ -429,20 +428,20 @@ def test_cli_manage_roles_exceptions(self, create_user_test4, action, role, mess
"email": TEST_USER2_EMAIL,
"roles": [],
},
- "Error: Can't load user \"{'username': 'imported_user2', 'lastname': 'doe2', "
- "'firstname': 'jon', 'email': 'test-user2@example.com', 'roles': []}\". \nDetails:{'roles': "
- "['Shorter than minimum length 1.']}",
+ "Error: Input file didn't pass validation. See below:\n"
+ "{0: {'roles': ['Shorter than minimum length 1.']}}",
],
[
{
- "username": "imported_user3",
+ "username1": "imported_user3",
"lastname": "doe3",
"firstname": "jon",
"email": TEST_USER3_EMAIL,
},
- "Error: Can't load user \"{'username': 'imported_user3', 'lastname': 'doe3', "
- "'firstname': 'jon', 'email': 'test-user3@example.com'}\". \nDetails:{'roles': "
- "['Missing data for required field.']}",
+ "Error: Input file didn't pass validation. See below:\n"
+ "{0: {'username': ['Missing data for required field.'], "
+ "'roles': ['Missing data for required field.'], "
+ "'username1': ['Unknown field.']}}",
Review comment:
I think this is too raw and we need to format the message slightly, something like
```
Error: Input file did not pass validation.
[Item 0]
username: Missing data for required field.
roles: Missing data for required field.
username1: Unknown field.
```
Also need to account for
1. When the input is not a list.
2. When an item in the list is not a dict.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r732931445
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -197,7 +197,13 @@ def _import_users(users_list: List[Dict[str, Any]]):
try:
UserSchema(many=True).load(users_list)
except ValidationError as e:
- raise SystemExit(f"Error: Input file didn't pass validation. See below:\n{e}")
+ msg = []
+ failures = e.messages
+ for row_num in failures:
+ msg.append(f'[Item {row_num}]')
+ for key in failures[row_num]:
+ msg.append(f'\t{key}: {failures[row_num][key]}')
Review comment:
Sorry, just pushed one just before your suggestion. Can you please suggest on the new one or shall I update?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r732892070
##########
File path: tests/cli/commands/test_user_command.py
##########
@@ -429,20 +428,20 @@ def test_cli_manage_roles_exceptions(self, create_user_test4, action, role, mess
"email": TEST_USER2_EMAIL,
"roles": [],
},
- "Error: Can't load user \"{'username': 'imported_user2', 'lastname': 'doe2', "
- "'firstname': 'jon', 'email': 'test-user2@example.com', 'roles': []}\". \nDetails:{'roles': "
- "['Shorter than minimum length 1.']}",
+ "Error: Input file didn't pass validation. See below:\n"
+ "{0: {'roles': ['Shorter than minimum length 1.']}}",
],
[
{
- "username": "imported_user3",
+ "username1": "imported_user3",
"lastname": "doe3",
"firstname": "jon",
"email": TEST_USER3_EMAIL,
},
- "Error: Can't load user \"{'username': 'imported_user3', 'lastname': 'doe3', "
- "'firstname': 'jon', 'email': 'test-user3@example.com'}\". \nDetails:{'roles': "
- "['Missing data for required field.']}",
+ "Error: Input file didn't pass validation. See below:\n"
+ "{0: {'username': ['Missing data for required field.'], "
+ "'roles': ['Missing data for required field.'], "
+ "'username1': ['Unknown field.']}}",
Review comment:
Fixed the output format.
re:
1. This is handled by JSON loader.
2. Added generic check test case
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-1003218137
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.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r780840451
##########
File path: tests/test_utils/api_connexion_utils.py
##########
@@ -110,6 +110,11 @@ def delete_user(app, username):
break
+def delete_users(app):
Review comment:
Is there a reason to have this function under `api_connexion_utils` when its only used in the CLI tests?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r718922743
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -159,27 +160,32 @@ def users_import(args):
if not os.path.exists(json_file):
raise SystemExit(f"File '{json_file}' does not exist")
- users_list = None
+ users_list_ = None
try:
with open(json_file) as file:
- users_list = json.loads(file.read())
+ users_list_ = json.loads(file.read())
except ValueError as e:
raise SystemExit(f"File '{json_file}' is not valid JSON. Error: {e}")
- users_created, users_updated = _import_users(users_list)
+ users_created, users_updated = _import_users(users_list_)
if users_created:
print("Created the following users:\n\t{}".format("\n\t".join(users_created)))
if users_updated:
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list_: List[Dict[str, Any]]):
appbuilder = cached_app().appbuilder
users_created = []
users_updated = []
- for user in users_list:
+ for user in users_list_:
+ required_fields = ['username', 'firstname', 'lastname', 'email', 'roles']
+ for field in required_fields:
+ if field not in user:
Review comment:
So this will still permit the creation of a user with `roles=[]` without erroring. Is that ok, or was the rejection of a user with no roles intentional?
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -159,27 +160,32 @@ def users_import(args):
if not os.path.exists(json_file):
raise SystemExit(f"File '{json_file}' does not exist")
- users_list = None
+ users_list_ = None
try:
with open(json_file) as file:
- users_list = json.loads(file.read())
+ users_list_ = json.loads(file.read())
except ValueError as e:
raise SystemExit(f"File '{json_file}' is not valid JSON. Error: {e}")
- users_created, users_updated = _import_users(users_list)
+ users_created, users_updated = _import_users(users_list_)
if users_created:
print("Created the following users:\n\t{}".format("\n\t".join(users_created)))
if users_updated:
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list_: List[Dict[str, Any]]):
appbuilder = cached_app().appbuilder
users_created = []
users_updated = []
- for user in users_list:
+ for user in users_list_:
+ required_fields = ['username', 'firstname', 'lastname', 'email', 'roles']
+ for field in required_fields:
+ if field not in user:
Review comment:
So this will still permit the creation of a user with `roles=[]` without erroring. Is that OK, or was the rejection of a user with no roles intended?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-945666104
> Since Marshmallow is already a hard dependency, I feel we should just use it for validation instead of hand-rolling logic.
Just tried to use UserCollectionItemSchema for validation and fails as `users_export` function removes underscores from first_name and last_name during export. This causes mismatch with UserCollectionItemSchema during load.
No sure why this is done:
https://github.com/apache/airflow/blob/86a2a19ad2bdc87a9ad14bb7fde9313b2d7489bb/airflow/cli/commands/user_command.py#L135-L148
Removing that will create incompatibility issue with import users feature. Perhaps we need a separate issue/goal for converting these type of codes to use schemas and discuss braking changes accordingly?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r719102947
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -159,27 +160,32 @@ def users_import(args):
if not os.path.exists(json_file):
raise SystemExit(f"File '{json_file}' does not exist")
- users_list = None
+ users_list_ = None
try:
with open(json_file) as file:
- users_list = json.loads(file.read())
+ users_list_ = json.loads(file.read())
except ValueError as e:
raise SystemExit(f"File '{json_file}' is not valid JSON. Error: {e}")
- users_created, users_updated = _import_users(users_list)
+ users_created, users_updated = _import_users(users_list_)
if users_created:
print("Created the following users:\n\t{}".format("\n\t".join(users_created)))
if users_updated:
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list_: List[Dict[str, Any]]):
appbuilder = cached_app().appbuilder
users_created = []
users_updated = []
- for user in users_list:
+ for user in users_list_:
+ required_fields = ['username', 'firstname', 'lastname', 'email', 'roles']
+ for field in required_fields:
+ if field not in user:
Review comment:
That's correct, I was thinking about that and missed that part, was very late... it would still work if we changed that back to dict.get but that would be implicit check and would yield misleading error about missing field. I will add explicit condition to check for an empty roles and add relevant test case
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-930489344
Fixed
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-1010356958
@potiuk Can you please review?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r732640318
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -174,12 +189,19 @@ def users_import(args):
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list: List[Dict[str, Any]]):
appbuilder = cached_app().appbuilder
users_created = []
users_updated = []
for user in users_list:
+
+ try:
+ # Validate structure
+ UserSchema().load(user)
+ except ValidationError as e:
+ raise SystemExit(f"Error: Can't load user \"{user}\". \nDetails:{e}")
Review comment:
Agreed. Added whole file validation before starting the import. Updated tests accordingly
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-930474988
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-947820123
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-945366853
Since Marshmallow is already a hard dependency, I feel we should just use it for validation instead of hand-rolling logic.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r719102947
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -159,27 +160,32 @@ def users_import(args):
if not os.path.exists(json_file):
raise SystemExit(f"File '{json_file}' does not exist")
- users_list = None
+ users_list_ = None
try:
with open(json_file) as file:
- users_list = json.loads(file.read())
+ users_list_ = json.loads(file.read())
except ValueError as e:
raise SystemExit(f"File '{json_file}' is not valid JSON. Error: {e}")
- users_created, users_updated = _import_users(users_list)
+ users_created, users_updated = _import_users(users_list_)
if users_created:
print("Created the following users:\n\t{}".format("\n\t".join(users_created)))
if users_updated:
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list_: List[Dict[str, Any]]):
appbuilder = cached_app().appbuilder
users_created = []
users_updated = []
- for user in users_list:
+ for user in users_list_:
+ required_fields = ['username', 'firstname', 'lastname', 'email', 'roles']
+ for field in required_fields:
+ if field not in user:
Review comment:
That's correct, I was thinking about that and missed that part, was very late... it would still work if ww changed that back to dict.get but that would be implicit check and would yoeld misleading error about missing tole field. Will add explicit condition to check for an empty roles and add relevant test case
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r718922743
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -159,27 +160,32 @@ def users_import(args):
if not os.path.exists(json_file):
raise SystemExit(f"File '{json_file}' does not exist")
- users_list = None
+ users_list_ = None
try:
with open(json_file) as file:
- users_list = json.loads(file.read())
+ users_list_ = json.loads(file.read())
except ValueError as e:
raise SystemExit(f"File '{json_file}' is not valid JSON. Error: {e}")
- users_created, users_updated = _import_users(users_list)
+ users_created, users_updated = _import_users(users_list_)
if users_created:
print("Created the following users:\n\t{}".format("\n\t".join(users_created)))
if users_updated:
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list_: List[Dict[str, Any]]):
appbuilder = cached_app().appbuilder
users_created = []
users_updated = []
- for user in users_list:
+ for user in users_list_:
+ required_fields = ['username', 'firstname', 'lastname', 'email', 'roles']
+ for field in required_fields:
+ if field not in user:
Review comment:
So this will still permit the creation of a user with `roles=[]` without erroring. Is that OK, or was the rejection of a user with no roles intended?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-1053733893
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.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-1054822130
I believe this is ready, just needs someone to review.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r782522821
##########
File path: tests/test_utils/api_connexion_utils.py
##########
@@ -110,6 +110,11 @@ def delete_user(app, username):
break
+def delete_users(app):
Review comment:
This was added with a view that to be used later in other suites where similar issues happens when users are created and never deleted.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #18590:
URL: https://github.com/apache/airflow/pull/18590
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-945839024
Or we can create a new schema specifically for CLI and keep everything compatible.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r732935344
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -174,12 +189,23 @@ def users_import(args):
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list: List[Dict[str, Any]]):
appbuilder = cached_app().appbuilder
users_created = []
users_updated = []
+ try:
+ UserSchema(many=True).load(users_list)
+ except ValidationError as e:
+ msg = []
+ for row_num, failure in e.messages.items():
+ msg.append(f'[Item {row_num}]')
+ for key in failure:
+ msg.append(f'\t{key}: {failure[key]}')
Review comment:
```suggestion
for key, value in failure.items():
msg.append(f'\t{key}: {value}')
```
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-1004154002
I think this is ready but need some love to get this merged. Anyone interested in taking a look?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r718922743
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -159,27 +160,32 @@ def users_import(args):
if not os.path.exists(json_file):
raise SystemExit(f"File '{json_file}' does not exist")
- users_list = None
+ users_list_ = None
try:
with open(json_file) as file:
- users_list = json.loads(file.read())
+ users_list_ = json.loads(file.read())
except ValueError as e:
raise SystemExit(f"File '{json_file}' is not valid JSON. Error: {e}")
- users_created, users_updated = _import_users(users_list)
+ users_created, users_updated = _import_users(users_list_)
if users_created:
print("Created the following users:\n\t{}".format("\n\t".join(users_created)))
if users_updated:
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list_: List[Dict[str, Any]]):
appbuilder = cached_app().appbuilder
users_created = []
users_updated = []
- for user in users_list:
+ for user in users_list_:
+ required_fields = ['username', 'firstname', 'lastname', 'email', 'roles']
+ for field in required_fields:
+ if field not in user:
Review comment:
So this will still permit the creation of a user with `roles=[]` without erroring. Is that ok, or was the rejection of a user with no roles intentional?
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#issuecomment-932222097
Update: Fixing this issue creates test failure in other parts. These are due to other tests create users with empty roles for testing and dont clean them up once done. I had to star fixing them ( #18667 and #18586) first before I can finish and merge this one
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r732926102
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -197,7 +197,13 @@ def _import_users(users_list: List[Dict[str, Any]]):
try:
UserSchema(many=True).load(users_list)
except ValidationError as e:
- raise SystemExit(f"Error: Input file didn't pass validation. See below:\n{e}")
+ msg = []
+ failures = e.messages
+ for row_num in failures:
+ msg.append(f'[Item {row_num}]')
+ for key in failures[row_num]:
+ msg.append(f'\t{key}: {failures[row_num][key]}')
Review comment:
```suggestion
for row_num, row_failures in failures.items():
msg.append(f'[Item {row_num}]')
for key, failure in row_failures.items():
msg.append(f'\t{key}: {failure}')
```
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r732935344
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -174,12 +189,23 @@ def users_import(args):
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list: List[Dict[str, Any]]):
appbuilder = cached_app().appbuilder
users_created = []
users_updated = []
+ try:
+ UserSchema(many=True).load(users_list)
+ except ValidationError as e:
+ msg = []
+ for row_num, failure in e.messages.items():
+ msg.append(f'[Item {row_num}]')
+ for key in failure:
+ msg.append(f'\t{key}: {failure[key]}')
Review comment:
```suggestion
for key, value in failure:
msg.append(f'\t{key}: {value}')
```
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r732938872
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -159,27 +160,32 @@ def users_import(args):
if not os.path.exists(json_file):
raise SystemExit(f"File '{json_file}' does not exist")
- users_list = None
+ users_list_ = None
try:
with open(json_file) as file:
- users_list = json.loads(file.read())
+ users_list_ = json.loads(file.read())
except ValueError as e:
raise SystemExit(f"File '{json_file}' is not valid JSON. Error: {e}")
- users_created, users_updated = _import_users(users_list)
+ users_created, users_updated = _import_users(users_list_)
if users_created:
print("Created the following users:\n\t{}".format("\n\t".join(users_created)))
if users_updated:
print("Updated the following users:\n\t{}".format("\n\t".join(users_updated)))
-def _import_users(users_list):
+def _import_users(users_list_: List[Dict[str, Any]]):
appbuilder = cached_app().appbuilder
users_created = []
users_updated = []
- for user in users_list:
+ for user in users_list_:
+ required_fields = ['username', 'firstname', 'lastname', 'email', 'roles']
+ for field in required_fields:
+ if field not in user:
Review comment:
Concerns addressed, resolving
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] khalidmammadov commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r782521989
##########
File path: tests/cli/commands/test_user_command.py
##########
@@ -45,18 +48,9 @@ def _set_attrs(self, app, dagbag, parser):
self.dagbag = dagbag
self.parser = parser
self.appbuilder = self.app.appbuilder
- self.clear_roles_and_roles()
+ delete_users(app)
Review comment:
Int this test suite no new/test roles are created and hence no need to delete any roles.
Besides, `clear_roles_and_roles` function is used inside `test_role_command.py` suite where it only deletes specific users and roles and when new users are added or imported here they were not deleted and hence this PR cleans those traces.
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #18590: Fixing bug when roles list is empty
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18590:
URL: https://github.com/apache/airflow/pull/18590#discussion_r820272922
##########
File path: airflow/cli/commands/user_command.py
##########
@@ -22,13 +22,28 @@
import random
import re
import string
+from typing import Any, Dict, List
+
+from marshmallow import Schema, fields, validate
+from marshmallow.exceptions import ValidationError
from airflow.cli.simple_table import AirflowConsole
from airflow.utils import cli as cli_utils
from airflow.utils.cli import suppress_logs_and_warning
from airflow.www.app import cached_app
+class UserSchema(Schema):
Review comment:
Nice to use marshmallow !
--
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-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org