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/10/01 11:22:05 UTC
[GitHub] [airflow] khalidmammadov opened a new pull request #18667: Cleaning roles and users for tests cases after completion
khalidmammadov opened a new pull request #18667:
URL: https://github.com/apache/airflow/pull/18667
This is to clean up test database once tests are completed as some test create users and roles and don't delete them. This affects other test suites when run in different scenarios like `pytest tests` to test all. For example one test adds a user 'test' and doesn't delete it. Then some other test author creates a 'test' user as well. This causes issues.
This PR fixes these issues in this file only and I'll file some more PRs to fix them all so they "leave the house tidy once leave"
This PR linked to #18653 and #18586 I have been working on
---
**^ 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 #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#discussion_r720790326
##########
File path: tests/www/test_security.py
##########
@@ -87,7 +87,7 @@ def setUpClass(cls):
cls.appbuilder = cls.app.appbuilder
cls.app.config['WTF_CSRF_ENABLED'] = False
cls.security_manager = cls.appbuilder.sm
- cls.delete_roles()
+ cls.delete_test_roles()
Review comment:
Thanks for the clarification.
--
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 #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#discussion_r720785585
##########
File path: tests/www/test_security.py
##########
@@ -87,7 +87,7 @@ def setUpClass(cls):
cls.appbuilder = cls.app.appbuilder
cls.app.config['WTF_CSRF_ENABLED'] = False
cls.security_manager = cls.appbuilder.sm
- cls.delete_roles()
+ cls.delete_test_roles()
Review comment:
Just a rename. Otherwise, it looks like a some important api function.
Rename makes it clear that this is just a test suites related func.
--
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 #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#discussion_r720783766
##########
File path: tests/www/test_security.py
##########
@@ -87,7 +87,7 @@ def setUpClass(cls):
cls.appbuilder = cls.app.appbuilder
cls.app.config['WTF_CSRF_ENABLED'] = False
cls.security_manager = cls.appbuilder.sm
- cls.delete_roles()
+ cls.delete_test_roles()
Review comment:
Is this only a simple rename or is `delete_roles` not technically doable?
--
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 #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#discussion_r720793674
##########
File path: tests/www/test_security.py
##########
@@ -99,14 +99,33 @@ def setUp(self):
log.debug("Complete setup!")
@classmethod
- def delete_roles(cls):
+ def tearDownClass(cls):
+ cls.delete_test_roles()
+ cls.delete_test_users()
+
+ @classmethod
+ def delete_test_users(cls):
+ method_users = [attr[5:] for attr in cls.__dict__ if attr[:4] == 'test']
+ adhoc_users = [
+ 'ElUser',
+ 'Monsieur User',
+ 'dag_access_user',
+ 'LaUser',
+ 'dag_permission_user'
+ ]
Review comment:
Here #18653
--
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 #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#discussion_r720816747
##########
File path: tests/www/test_security.py
##########
@@ -99,14 +99,33 @@ def setUp(self):
log.debug("Complete setup!")
@classmethod
- def delete_roles(cls):
+ def tearDownClass(cls):
+ cls.delete_test_roles()
+ cls.delete_test_users()
+
+ @classmethod
+ def delete_test_users(cls):
+ method_users = [attr[5:] for attr in cls.__dict__ if attr[:4] == 'test']
+ adhoc_users = [
+ 'ElUser',
+ 'Monsieur User',
+ 'dag_access_user',
+ 'LaUser',
+ 'dag_permission_user'
+ ]
Review comment:
This is just an incremental change to fix tidy up issue that left by the test. But if it's better to refactoring of whole suite to pytest then it's fine, I can close this one can look into the refactoring and fixing it for the good. I was just trying to avoid big changes and not sure how community likes it or not. I can add a new suite with similar name with goal to phase out 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] potiuk commented on a change in pull request #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#discussion_r720917367
##########
File path: tests/www/test_security.py
##########
@@ -99,14 +99,33 @@ def setUp(self):
log.debug("Complete setup!")
@classmethod
- def delete_roles(cls):
+ def tearDownClass(cls):
+ cls.delete_test_roles()
+ cls.delete_test_users()
+
+ @classmethod
+ def delete_test_users(cls):
+ method_users = [attr[5:] for attr in cls.__dict__ if attr[:4] == 'test']
+ adhoc_users = [
+ 'ElUser',
+ 'Monsieur User',
+ 'dag_access_user',
+ 'LaUser',
+ 'dag_permission_user'
+ ]
Review comment:
I agree with @uranusjr - let's refactor it to pytest in one go as well. We are transitoning towards pytest fixtures now and it would be great to use it as an opportunity to do it here as well.
--
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 closed pull request #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
khalidmammadov closed pull request #18667:
URL: https://github.com/apache/airflow/pull/18667
--
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 #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#issuecomment-933262116
Closing this as per above discussion
--
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 #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#discussion_r720799486
##########
File path: tests/www/test_security.py
##########
@@ -99,14 +99,33 @@ def setUp(self):
log.debug("Complete setup!")
@classmethod
- def delete_roles(cls):
+ def tearDownClass(cls):
+ cls.delete_test_roles()
+ cls.delete_test_users()
+
+ @classmethod
+ def delete_test_users(cls):
+ method_users = [attr[5:] for attr in cls.__dict__ if attr[:4] == 'test']
+ adhoc_users = [
+ 'ElUser',
+ 'Monsieur User',
+ 'dag_access_user',
+ 'LaUser',
+ 'dag_permission_user'
+ ]
Review comment:
I was thinking about the same thing, this should use pytest fixtures to put creation and deletion in one place instead of listing the usernames twice. I'm probably missing some context why this PR is needed if you intend to convert the tests anyway and will leave this for others to comment on.
--
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 #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#discussion_r721146117
##########
File path: tests/www/test_security.py
##########
@@ -99,14 +99,33 @@ def setUp(self):
log.debug("Complete setup!")
@classmethod
- def delete_roles(cls):
+ def tearDownClass(cls):
+ cls.delete_test_roles()
+ cls.delete_test_users()
+
+ @classmethod
+ def delete_test_users(cls):
+ method_users = [attr[5:] for attr in cls.__dict__ if attr[:4] == 'test']
+ adhoc_users = [
+ 'ElUser',
+ 'Monsieur User',
+ 'dag_access_user',
+ 'LaUser',
+ 'dag_permission_user'
+ ]
Review comment:
Ok, no worries, I will refactor
--
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 removed a comment on pull request #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
khalidmammadov removed a comment on pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#issuecomment-932881482
Just a rename. Otherwise, it looks like a some important api function.
Rename makes it clear that this is just a test suites related func.
On Sun, 3 Oct 2021, 08:22 Tzu-ping Chung, ***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In tests/www/test_security.py
> <https://github.com/apache/airflow/pull/18667#discussion_r720783766>:
>
> > - cls.delete_roles()
> + cls.delete_test_roles()
>
> Is this only a simple rename or is delete_roles not technically doable?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/airflow/pull/18667#pullrequestreview-769554181>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACYJ3NDHVIAPS7RQLEPZHLDUFAADPANCNFSM5FEO2RVQ>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
--
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 #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#discussion_r720790500
##########
File path: tests/www/test_security.py
##########
@@ -99,14 +99,33 @@ def setUp(self):
log.debug("Complete setup!")
@classmethod
- def delete_roles(cls):
+ def tearDownClass(cls):
+ cls.delete_test_roles()
+ cls.delete_test_users()
+
+ @classmethod
+ def delete_test_users(cls):
+ method_users = [attr[5:] for attr in cls.__dict__ if attr[:4] == 'test']
+ adhoc_users = [
+ 'ElUser',
+ 'Monsieur User',
+ 'dag_access_user',
+ 'LaUser',
+ 'dag_permission_user'
+ ]
Review comment:
Hmm, this fixed list makes me a bit uncomfortable. Why are those `adhoc_users` special? And is there a way to get `method_users` that does not involve magically inferring from name?
--
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 #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#issuecomment-932881482
Just a rename. Otherwise, it looks like a some important api function.
Rename makes it clear that this is just a test suites related func.
On Sun, 3 Oct 2021, 08:22 Tzu-ping Chung, ***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In tests/www/test_security.py
> <https://github.com/apache/airflow/pull/18667#discussion_r720783766>:
>
> > - cls.delete_roles()
> + cls.delete_test_roles()
>
> Is this only a simple rename or is delete_roles not technically doable?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/airflow/pull/18667#pullrequestreview-769554181>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACYJ3NDHVIAPS7RQLEPZHLDUFAADPANCNFSM5FEO2RVQ>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
>
--
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 #18667: Cleaning roles and users for tests cases after completion
Posted by GitBox <gi...@apache.org>.
khalidmammadov commented on a change in pull request #18667:
URL: https://github.com/apache/airflow/pull/18667#discussion_r720793471
##########
File path: tests/www/test_security.py
##########
@@ -99,14 +99,33 @@ def setUp(self):
log.debug("Complete setup!")
@classmethod
- def delete_roles(cls):
+ def tearDownClass(cls):
+ cls.delete_test_roles()
+ cls.delete_test_users()
+
+ @classmethod
+ def delete_test_users(cls):
+ method_users = [attr[5:] for attr in cls.__dict__ if attr[:4] == 'test']
+ adhoc_users = [
+ 'ElUser',
+ 'Monsieur User',
+ 'dag_access_user',
+ 'LaUser',
+ 'dag_permission_user'
+ ]
Review comment:
Adhoc users list is static and taken from this suite's test cases. They are created and never deleted. This func has the same logic (with static list) as existing delete_roles func. So I thought it makes them inline anyway.
Method users are there as there are tests that uses func name as a username, so instead of listing them all in here func tries to find those as well for deletion.
Ideally it would be better to have something like create_user_scope function like I have added in the other PR but that suite uses pytest api.
--
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