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

RFC - Customizations #2011

Closed
wants to merge 1 commit into from
Closed

RFC - Customizations #2011

wants to merge 1 commit into from

Conversation

0x0ece
Copy link
Contributor

@0x0ece 0x0ece commented Jan 20, 2017

Hi,

I'm proposing an initial plugins system.

The idea is to have a plugins directory containing mostly empty files, which are already "properly linked" in the code. This way, one can tweak Superset, but also easily upgrade to newer versions, with low-to-none merge conflicts. (This is heavily inspired by Airflow.)

At shopkick we're using superset since 0.10, did a lot of internal small patches, mostly personalizing the UI, and every time there's an update re-merging is a pretty painful operation.

With this current system, we're currently personalizing:

  • bootstrap variables, notably the brand color
  • other css
  • colors, very important for our internal/core metrics
  • additional personalized visualizations (probably too personalized to be added to the main branch)
  • views, with their own template, including static pages, additional api calls, etc.
  • generic python extensions, e.g. a security class that auto-register users if the email domain is the correct one (again, probably too personalized to be added in the main branch)

You can see an example of the plugins system in action here:
https://github.com/shopkick/superset/tree/sk_0.15.2/superset/plugins

To reiterate, the main purpose is to have empty files somewhere that one can personalize as she likes, without incurring in merge conflicts when a new version is released. In principle every single piece of Superset can be extended in a similar fashion, but I'm not a fan of too general implementations, I'd grow the list of supported features as the community requires them.

Please LMK what do you think, E.

@xrmx
Copy link
Contributor

xrmx commented Jan 20, 2017

On the python side i really don't like the let's import everything from a file approach. Airflow take on handling this looks quite a bit more sophisticated to me https://github.com/apache/incubator-airflow/blob/master/airflow/plugins_manager.py

@xrmx
Copy link
Contributor

xrmx commented Jan 20, 2017

BTW we now have something similar for custom datasources, see 5a0e06e and fc921d6

@alanmcruickshank
Copy link
Contributor

alanmcruickshank commented Jan 20, 2017 via email

@0x0ece
Copy link
Contributor Author

0x0ece commented Jan 20, 2017

Sounds good to me, we can think of config vars where one defines the list of things to import.
Any comment on the js part? That's the most annoying imo.

@mahmoud
Copy link

mahmoud commented Jan 20, 2017

@xrmx I think the difference in approaches can be traced to semantics. Airflow has more of a plugin system which offers extension for an installed instance. @0x0ece's feature may more properly be called a "customization" framework, suitable for those maintaining (and developing on) forks. Personally, I appreciate the functionality and simplicity of the approach.

@alanmcruickshank The import * antipattern is mostly referring to cases where the code goes on to use imported components by name. Programmatic import/usage like this is far less offensive, though I agree a full plugin system would have a more refined approach.

@0x0ece
Copy link
Contributor Author

0x0ece commented Jan 20, 2017

I agree with @mahmoud, that's a good point - I should rename the directory to custom instead of plugins. This is not intended, as in Airflow, to install Superset and load arbitrary code, but to recompile Superset with your code inside. The main reason being of course js.

@xrmx
Copy link
Contributor

xrmx commented Jan 20, 2017

@mahmoud i don't think the python side of this PR is about a plugin system as it's not about making the code extensible. It's just a shortcut to maintain forks easily.

@0x0ece thanks for sharing your customization, a few random thoughts:

  • the usa and europe map can use some code reuse and a generic way to express a subset of the world may be interesting for everyone
  • the custom fab security should imho be a different package and just a configuration for your installation, not something you'll be shipping with superset
  • for the templates you should use something like this i think https://buxty.com/b/2012/05/custom-template-folders-with-flask/
  • the color configuration changes seems legit (and would make a lot of people happy)

@0x0ece
Copy link
Contributor Author

0x0ece commented Jan 20, 2017

All right, let's recap then:

  • I'll rename plugins into custom, so the meaning is more clear
  • I'll avoid import * in favor of a config à la data sources for views and viz

I'll wait for a comment on the js from the FE people, e.g. @ascott, and post an update asap.

As for my specific example:

  • we can certainly extract some stuff from the maps and improve the world map, but this will be an independent PR (if anyone wants to jump on it, feel free!)
  • custom security and templates can be placed wherever one likes, not necessarily inside the custom -- for me it's a simplification so I can manage everything in a single place
  • in short, no action wrt to this PR

@0x0ece 0x0ece changed the title RFC - Plugins RFC - Customizations Jan 20, 2017
@0x0ece
Copy link
Contributor Author

0x0ece commented Jan 20, 2017

@alanmcruickshank BTW, we were using something around these lines: #1188
but don't have the bandwidth now to port look into that and how it evolved.

@mistercrunch
Copy link
Member

This is all pretty reasonable, though the codebase may get shuffled around quite a bit still (before 1.0) and I'd prefer not having to worry about keeping this "plugins interface" working while refactoring.

For instance, we probably want to refactor the visualizations so that each one gets a folder with there related .py, .js and .css. Similarly, I was planning on refactoring the "connectors" (druid and sqlalchemy) out of models.py and view.py.

If we hook up the plugins as defined in this PR, it will make that refactoring harder if not impossible....

@0x0ece
Copy link
Contributor Author

0x0ece commented Jan 24, 2017

All right, I'm closing this PR then. But please think of a way for simple contributions/personalization in the main refactor. :)

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