-
Notifications
You must be signed in to change notification settings - Fork 13.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Remove deprecated ENABLE_ACCESS_REQUEST workflow #24266
chore: Remove deprecated ENABLE_ACCESS_REQUEST workflow #24266
Conversation
4b422f7
to
a7c6abf
Compare
__(security_manager.get_datasource_access_error_msg(datasource)), | ||
"danger", | ||
) | ||
return redirect( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this redirect no longer worked as this is being called via a GET
yet the redirect URL only accepts a POST
.
a7c6abf
to
eeea8c9
Compare
Codecov Report
@@ Coverage Diff @@
## master #24266 +/- ##
========================================
Coverage 69.08% 69.09%
========================================
Files 1905 1903 -2
Lines 74813 74608 -205
Branches 8107 8107
========================================
- Hits 51686 51550 -136
+ Misses 21016 20947 -69
Partials 2111 2111
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
f949506
to
8c9dd8b
Compare
8c9dd8b
to
fb6f391
Compare
9e825ba
to
44f918b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that probably makes sense to remove /superset/override_role_permissions
also, since it seems related with is granter
role. See if you agree and I'll close: #24334
f2f9cb5
to
17b5020
Compare
@dpgaspar I agree with your comment as I went down this path previously but didn't know how far I should go. This also makes the |
d2be248
to
6511cb2
Compare
@@ -348,7 +348,7 @@ def test_get_dataset_item(self): | |||
"sql": None, | |||
"table_name": "energy_usage", | |||
"template_params": None, | |||
"uid": "2__table", | |||
"uid": "6__table", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love how removing some tests exposed the non-idempotent facets of others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're only asserting fields that are listed here (see lines 375-377), maybe we can just remove this line? I'm not sure we really need to assert the uid
, as I assume doing this assertion will fail when running this test in isolation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll use mock.ANY
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this dead code being removed, as this has caused a lot of confusion over the years. If this type of functionality really is needed, it should probably be done slightly differently, using pluggable FAB views/classes etc that don't require this much logic in the main codebase.
@@ -348,7 +348,7 @@ def test_get_dataset_item(self): | |||
"sql": None, | |||
"table_name": "energy_usage", | |||
"template_params": None, | |||
"uid": "2__table", | |||
"uid": "6__table", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're only asserting fields that are listed here (see lines 375-377), maybe we can just remove this line? I'm not sure we really need to assert the uid
, as I assume doing this assertion will fail when running this test in isolation.
726adbc
to
2d82c46
Compare
2d82c46
to
dc2a4d1
Compare
@john-bodley just to be on the safe side, I'm going to mark this as a possible breaking change. Do you think we should add anything to the UPDATING.md file re the changes? |
SUMMARY
This PR partially addresses Superset 3.0 task #9 by removing the previous deprecated
ENABLE_ACCESS_REQUEST
config parameter and the associated workflows.The TL;DR is this legacy workflow failed to work post #22022 (which mentions that the API endpoint is deprecated and will be removed in 3.0) when the
GET
requests becamePOST
requests meaning logic like this and this is currently broken.Honestly I'm going in somewhat blind in terms of the how/where this logic was used, but the TL;DR is there seems to be no approval logic/workflow if there's no request workflows. Hopefully I've trimmed enough dead code without over pruning, though this has turned somewhat into a yak shaving exercise as—by deleting the
/approve
endpoint—it led to removing facets of role extension, deleting thegranter
role, etc.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION