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

fix: Add all Trino dependencies for Kerberos enablement #26077

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Nov 22, 2023

SUMMARY

Per our documentation, Superset supports Kerberos authentication for Trino, however installing the superset[trino] dependencies are not suffice as the trino package additionally requires the requests-kerberos package.

Rather than defining trino[kerberos] as a dependency I though it would be prudent to defining trino[all] as this contains additional requirements like SQLAlchemy.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Ran pip-compile-multi --no-upgrade.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@john-bodley john-bodley changed the title fix: Add all Trino dependencies to Kerberos enablement fix: Add all Trino dependencies for Kerberos enablement Nov 22, 2023
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -371,6 +369,7 @@ werkzeug==2.3.3
# via
# apache-superset
# flask
# flask-appbuilder
Copy link
Member

@villebro villebro Nov 23, 2023

Choose a reason for hiding this comment

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

I think we need to add a CI linting rule to make sure the requirements files are always up to date (I assume this PR didn't bring about this change). Any objections to me adding a step to the python lint CI workflow that does this in a separate PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

@villebro I agree. We do have this check, however this just makes sure that the SHAs are correct. Maybe we need to run pip-compile-multi --no-upgrade and make sure there's no resulting diff?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we need to run pip-compile-multi --no-upgrade and make sure there's no resulting diff?

@john-bodley yeah, I was thinking something along those lines

@john-bodley john-bodley force-pushed the john-bodley--requirements-trino-all branch from 01311cb to 05ad639 Compare November 28, 2023 17:13
Copy link

codecov bot commented Nov 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6447529) 69.10% compared to head (639fd80) 69.10%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #26077   +/-   ##
=======================================
  Coverage   69.10%   69.10%           
=======================================
  Files        1940     1940           
  Lines       75869    75869           
  Branches     8445     8445           
=======================================
+ Hits        52427    52428    +1     
+ Misses      21267    21266    -1     
  Partials     2175     2175           
Flag Coverage Δ
hive 53.68% <ø> (ø)
javascript 56.28% <ø> (ø)
mysql 78.13% <ø> (-0.03%) ⬇️
postgres 78.25% <ø> (ø)
presto 53.63% <ø> (ø)
python 82.95% <ø> (+<0.01%) ⬆️
sqlite 76.91% <ø> (ø)
unit 55.79% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@john-bodley
Copy link
Member Author

Closing in favor of #26119 as we likely don't want to bloat our CI dependencies when there is an alternative workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants