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

Superset ROCK for review #12

Open
wants to merge 1 commit into
base: rock-review-base
Choose a base branch
from
Open

Conversation

AmberCharitos
Copy link
Collaborator

PR for review of the Superset ROCK.

Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

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

I was just reviewing the rockcraft.yaml (saw the request in the Rocks channel). It looks quite nice! I just left two very minor comments.

Comment on lines +17 to +25
override: replace
summary: "superset-ui service"
startup: disabled
command: "/app/k8s/k8s-bootstrap.sh"
environment:
CHARM_FUNCTION: "app-gunicorn"
SUPERSET_SECRET_KEY: "supersetR0cks!"
ADMIN_PASSWORD: "admin"
SUPERSET_LOAD_EXAMPLES: True
Copy link
Member

Choose a reason for hiding this comment

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

Note that the on-success and on-failure actions are restart by default. See Pebble readme.

You might want to make that explicit in the rockcraft.yaml to avoid any surprises.

Comment on lines +114 to +132
# Required dependencies missing from Superset requirements.txt
pip install Pillow==10.1.0
pip install requests==2.31.0
pip install jmespath==1.0.1

# Optional dependencies for Database connectors
pip install psycopg2-binary==2.9.4
pip install sqlalchemy==1.4.51
pip install Werkzeug==2.3.7
pip install Authlib==1.2.1
pip install elasticsearch-dbapi==0.2.10
pip install trino==0.327.0
pip install pyhive==0.7.0
pip install thrift==0.16.0
pip install sqlalchemy-redshift==0.8.1
pip install urllib3==1.26.11

# Monitoring
pip install sentry-sdk==0.10.2
Copy link
Member

Choose a reason for hiding this comment

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

nit: should you keep them in file(s)?

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.

2 participants