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

Upgrade to apache-superset==3.1.0 #33

Merged
merged 8 commits into from
Jan 29, 2024
Merged

Upgrade to apache-superset==3.1.0 #33

merged 8 commits into from
Jan 29, 2024

Conversation

kaapstorm
Copy link
Contributor

Upgrades hq_superset to apache-superset==3.1.0

This required a few tweaks:

  • superset.dataset.commands moved to superset.commands.dataset
  • is_user_admin() became security_manager.is_admin()
  • get_sqla_engine() method became get_sqla_engine_with_context() context manager.
  • The parameters of a couple of commands changed

That was about it.

setup.py Outdated
'jinja2==3.1.2',
'psycopg2==2.9.6',
'requests==2.31.0',
'sentry-sdk==1.39.2',

Choose a reason for hiding this comment

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

If we use sentry-sdk[flask], then we don't have to install blinker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

@SmittieC SmittieC left a comment

Choose a reason for hiding this comment

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

This is gold, thanks @kaapstorm

@Charl1996
Copy link
Contributor

Nice!

@kaapstorm kaapstorm merged commit e579250 into master Jan 29, 2024
3 checks passed
@kaapstorm kaapstorm deleted the nh/superset_3.1.0 branch January 29, 2024 11:13
'WTForms==2.3.3',
'cryptography==37.0.4',
'sentry-sdk==1.9.10',
'apache-superset==3.1.0',
Copy link
Member

@sravfeyn sravfeyn Jan 31, 2024

Choose a reason for hiding this comment

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

Shouldn't we be using our forked superset? This would break some of the features we have implemented already. Specifically this dimagi/superset#1 (which allows users to see UCR name instead of the data table UUID in List Data Sources page)

Copy link
Member

@sravfeyn sravfeyn Jan 31, 2024

Choose a reason for hiding this comment

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

There is few docs I added that explain how to build an updated forked package ahttps://github.com/dimagi/hq_superset/blob/master/apache-superset.md
We would need to use the fork repo instead of the core repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback!

This resolves some of my confusion: As you can see from the master branch of dimagi-superset, there is only one commit difference between it and apache-superset. And that commit has some Portuguese translations (which cannot be rebased on top of Superset 3.1.0 because of unresolveable conflicts).

It introduces more questions though. I'll follow them up offline. I'm very sorry that I missed that documentation!

Copy link
Contributor

Choose a reason for hiding this comment

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

hey @kaapstorm
Just confirming that we are now using the apache superset only or its a temporary change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a temporary change. I have uploaded dimagi-superset 3.1.0 to PyPI just now, and I'll update these requirements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#36

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

Successfully merging this pull request may close these issues.

5 participants