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
add kerberos for druid #7091
add kerberos for druid #7091
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7091 +/- ##
==========================================
- Coverage 64.61% 64.58% -0.04%
==========================================
Files 422 422
Lines 20607 20624 +17
Branches 2253 2253
==========================================
+ Hits 13315 13319 +4
- Misses 7170 7183 +13
Partials 122 122
Continue to review full report at Codecov.
|
what can`t be done ? |
? |
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.
Needs a few tests around Kerberos stuff. Ideally, the fetching of datasources would be broken out of the "model" and would live in another layer.
return False | ||
|
||
kerberos_commands = 'kinit -k -t %s %s' % (kerberos_keytab, kerberos_principal) | ||
subprocess.call(kerberos_commands, shell=True) |
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.
Not sure I feel comfortable shelling out like this, from within a model, no less. This sort of logic belongs in a higher layer (connection manager, perhaps?)
|
||
def get_druid_version(self): | ||
|
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.
extra space?
@@ -72,3 +72,4 @@ webencodings==0.5.1 # via bleach | |||
werkzeug==0.14.1 # via flask | |||
wtforms-json==0.3.3 | |||
wtforms==2.2.1 # via flask-wtf, wtforms-json | |||
requests-kerberos==0.12.0 |
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.
This should be in extras
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.
+1 let's create a kerberos group in extra_requires
Any news on this pull request? |
Looks like there's merge conflicts, and there were a few comments that weren't addressed. |
FYI, the Druid connector is getting deprecated [slowly over time] in favor of using SQLAlchemy and the SQL interface to Druid. We can still merge this once the comments are addressed, but the core committers will fade out of supporting the native Druid connector. This aligns with the Druid roadmap where they're clearly doubling down on the SQL interface |
add kerberos configure for Druid