onaio / superset-patchup

Superset-patchup is a python package that "patches" Superset to add custom functionality that we find to be useful
https://canopyinsights.com
Apache License 2.0
8 stars 7 forks source link

Add the ability to configure javascript allowed #10

Closed moshthepitt closed 5 years ago

moshthepitt commented 5 years ago

Make it possible the ability to configure javascript allowed for Superset i.e. set ENABLE_JAVASCRIPT_CONTROLS to True

pld commented 5 years ago

Can't this go in the playbook?

On Thu, Feb 14, 2019 at 05:54 Kelvin Jayanoris notifications@github.com wrote:

Make it possible the ability to configure javascript allowed for Superset i.e. set ENABLE_JAVASCRIPT_CONTROLS to True

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/onaio/superset-patchup/issues/10, or mute the thread https://github.com/notifications/unsubscribe-auth/AADGEgjgY3I7-zNhPXJ6k8kJnlu0UxPeks5vNUBhgaJpZM4a7VYF .

moshthepitt commented 5 years ago

@pld yes it can and I think it already is in the playbook.

But we need it here as well as this is the tool that we will be using to create a nicer superset_config.py file for the docker-based deployment and I am hoping that at some point it will be used in the playbook/role.

This is an issue that better describes the goal here: https://github.com/onaio/canopy/issues/172

pld commented 5 years ago

I don't understand why we'd need it here if it's in the role and we're going to be using the role in the future to do the docker-based deploys

moshthepitt commented 5 years ago

Some of the changes that we are doing to Superset include things such as adding views e.g. the CSV slice view that @Wambere added a few weeks ago. We might do more and more of these.

These kinds of "patches" can't be handled well by the playbooks/role alone in my view.

So that is why this repo exists, the main goals:

Does this make sense?

pld commented 5 years ago

Yes that makes sense, and I'm on board with that.

My comment is only about this specific issue, is there a reason why this specifically can't be handled by a change to the role?

moshthepitt commented 5 years ago

I think at least for a while longer we'll still be having some docker-based deploys of Superset that would need this, which would be an improvement over how it is done currently.

moshthepitt commented 5 years ago

The role already has this setting https://github.com/onaio/ansible-superset/pull/21

pld commented 5 years ago

Great, so my main point here is I don't want us duplicating what we have working in a role in a docker-build, I want us to commit to one, or the other, or an architecture that unifies a significant portion of the config overlap.

I.e. figure out the above instead of this issue, so I would consider this issue blocked.

moshthepitt commented 5 years ago

I think that makes sense.

I'm going to write up a clearer issue/comment/plan on how I see that this repo is part of the architecture that unifies a significant portion of the config overlap that we all want.

moshthepitt commented 5 years ago

@pld I'm going to close this issue - you were right, it is not to be done by superset patchup. It should be done during deployment or by a deployment script when installing superset.