Skip to content
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

Avoid circular dependency between superset config and superset package #8254

Merged

Conversation

bkyryliuk
Copy link
Member

CATEGORY

Choose one

  • Bug Fix

SUMMARY

Small refactor to avoid circular dependency between superset config and superset package.
Current code doesn't support adding custom security manager as importing SupersetSecurityManager calls init.py that initializes the config.

TEST PLAN

  • local testing + testing in DBX environment

REVIEWERS

@bkyryliuk bkyryliuk force-pushed the bogdan/support_custom_security_manager branch from 4c727fc to aa14dd3 Compare September 18, 2019 23:58
@dpgaspar
Copy link
Member

dpgaspar commented Sep 19, 2019

I would rename superset/security/security.py to superset/security/manager.py.
You need to add the Apache license to superset/security/__init__.py

A nice to have could be to import SupersetSecurityManager on the __init__.py, it would minimize any breaking changes

@bkyryliuk bkyryliuk force-pushed the bogdan/support_custom_security_manager branch 2 times, most recently from d05bb9d to 9dfb3dd Compare September 19, 2019 23:58
@codecov-io
Copy link

codecov-io commented Sep 20, 2019

Codecov Report

Merging #8254 into master will decrease coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8254      +/-   ##
==========================================
- Coverage   65.91%   65.78%   -0.13%     
==========================================
  Files         482      483       +1     
  Lines       24157    24311     +154     
  Branches     2770     2771       +1     
==========================================
+ Hits        15922    15992      +70     
- Misses       8057     8141      +84     
  Partials      178      178
Impacted Files Coverage Δ
superset/security/manager.py 84.13% <ø> (ø)
superset/security/__init__.py 100% <100%> (ø)
superset/connectors/connector_registry.py 87.5% <0%> (+0.83%) ⬆️
superset/cli.py 39.05% <0%> (+1.86%) ⬆️
...ssets/src/explore/components/AdhocFilterOption.jsx 61.53% <0%> (+3.2%) ⬆️
superset/common/query_context.py 29.05% <0%> (+3.66%) ⬆️
superset/connectors/base/views.py 80% <0%> (+8.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fb95cc...16bfea7. Read the comment docs.

@bkyryliuk bkyryliuk changed the title WIP. Avoid circular dependency between superset config and superset package Avoid circular dependency between superset config and superset package Sep 20, 2019
@bkyryliuk
Copy link
Member Author

@dpgaspar did you mean import SupersetSecurityManager in superset/security/__init__.py

@dpgaspar
Copy link
Member

Yep, just edited my previous reply

@bkyryliuk
Copy link
Member Author

@dpgaspar done

@@ -61,6 +61,9 @@ def _try_json_readfile(filepath):
return None


STATS_LOGGER = StatsdStatsLogger
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooops, it's not

@bkyryliuk bkyryliuk force-pushed the bogdan/support_custom_security_manager branch from a35c118 to c591ee0 Compare September 27, 2019 16:27
@bkyryliuk bkyryliuk force-pushed the bogdan/support_custom_security_manager branch from c591ee0 to 7bec333 Compare October 15, 2019 17:23
@bkyryliuk bkyryliuk force-pushed the bogdan/support_custom_security_manager branch from 7bec333 to 8196da9 Compare November 12, 2019 01:58
@bkyryliuk bkyryliuk force-pushed the bogdan/support_custom_security_manager branch 3 times, most recently from bdf4e0f to f731eae Compare December 5, 2019 23:21
Resolve comments

Avoid circular dependency between superset config and superset package

Resolve comments
@bkyryliuk bkyryliuk force-pushed the bogdan/support_custom_security_manager branch from f731eae to 16bfea7 Compare December 10, 2019 01:26
@dpgaspar dpgaspar merged commit e6be519 into apache:master Dec 10, 2019
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants