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/08 02:59:58 UTC
[GitHub] [airflow] uranusjr opened a new pull request #18820: Remove deprecated usage of init_role() from API
uranusjr opened a new pull request #18820:
URL: https://github.com/apache/airflow/pull/18820
Some cleanups found when investigating #18813.
`init_role()` has been replaced by `bulk_update_roles()`. This PR also slightly tweaked the ordering of operations so `bulk_sync_roles()` is only called when there's a need to update permissions, and added a few lines in the test to make sure role rename is performed successfully.
--
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] alex-astronomer edited a comment on pull request #18820: Remove deprecated usage of init_role() from API
Posted by GitBox <gi...@apache.org>.
alex-astronomer edited a comment on pull request #18820:
URL: https://github.com/apache/airflow/pull/18820#issuecomment-939184181
Not sure how to request permission to add to this PR but will you please add this test to confirm that #18813 was fixed?
```
def test_patch_updates_correct_roles_permissions(self):
"""
Issue described in #18813, fixed by #18820
"""
role_to_change = create_role(self.app, "patched_role")
existing_role = create_role(self.app, "already_exists")
assert len(role_to_change.permissions) == 0
assert len(existing_role.permissions) == 0
payload = {
"name": "already_exists",
"actions": [{"action": {"name": "can_delete"}, "resource": {"name": "XComs"}}],
}
response = self.client.patch(
f"/api/v1/roles/{role_to_change.name}", json=payload, environ_overrides={"REMOTE_USER": "test"}
)
assert response.status_code == 200
# have to use find_role because the session becomes detached during the test from the
# previous role definitions
assert len(self.app.appbuilder.sm.find_role("patched_role").permissions) == 1
assert len(self.app.appbuilder.sm.find_role("already_exists").permissions) == 0
expected_permission = role_to_change.permissions[0]
assert (expected_permission.permission.name, expected_permission.view_menu.name) == (
"can_delete",
"XComs",
)
```
This test fails on main but is corrected with #18820 (this PR)
--
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] alex-astronomer commented on pull request #18820: Remove deprecated usage of init_role() from API
Posted by GitBox <gi...@apache.org>.
alex-astronomer commented on pull request #18820:
URL: https://github.com/apache/airflow/pull/18820#issuecomment-939184181
Not sure how to request permission to add to this PR but will you please add this test to confirm that #18813 was fixed?
```
def test_patch_updates_correct_roles_permissions(self):
"""
Issue described in #18813, fixed by #18820
"""
role_to_change = create_role(self.app, "patched_role")
existing_role = create_role(self.app, "already_exists")
assert len(role_to_change.permissions) == 0
assert len(existing_role.permissions) == 0
payload = {
"name": "already_exists",
"actions": [{"action": {"name": "can_delete"}, "resource": {"name": "XComs"}}],
}
response = self.client.patch(
f"/api/v1/roles/{role_to_change.name}", json=payload, environ_overrides={"REMOTE_USER": "test"}
)
assert response.status_code == 200
# have to use find_role because the session becomes detached during the test from the
# previous role definitions
assert len(self.app.appbuilder.sm.find_role("patched_role").permissions) == 1
assert len(self.app.appbuilder.sm.find_role("already_exists").permissions) == 0
expected_permission = role_to_change.permissions[0]
assert (expected_permission.permission.name, expected_permission.view_menu.name) == (
"can_delete",
"XComs",
)
```
--
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 merged pull request #18820: Remove deprecated usage of init_role() from API
Posted by GitBox <gi...@apache.org>.
uranusjr merged pull request #18820:
URL: https://github.com/apache/airflow/pull/18820
--
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 #18820: Remove deprecated usage of init_role() from API
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #18820:
URL: https://github.com/apache/airflow/pull/18820#issuecomment-939140113
I just realised instead of `"name" not in data`, this should check for emptiness and equality like the user endpoint.
--
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 #18820: Remove deprecated usage of init_role() from API
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18820:
URL: https://github.com/apache/airflow/pull/18820#issuecomment-938353484
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 main 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