[GitHub] [superset] kristw edited a comment on issue #13013: [SIP-58] Proposal to move superset-ui to this repo

[GitBox <gi...@apache.org>]

kristw edited a comment on issue #13013:
URL: https://github.com/apache/superset/issues/13013#issuecomment-776617706


   When we moved to the separated repos, the goal was also to raise code quality bar, increase development velocity, and produce components that can be natively embedded in other applications (therefore the `npm` packages).
   However, as @ktmud mentioned, it has been 2 years since then and many things were changed.
   
   ----
   
   **History-1:** The quality of the code in `apache/(incubator-)superset` was not great. Some parts were well tested. Some not. Lints were enabled only for certain parts. `superset-ui` was created to be stricter: requiring 100% test coverage and TypeScript. We rewrote many core parts including massive clean up of all charts, then replaced legacy code in the main repo with these newer components. 
   
   **Current:** 🟢  Main repo has more TypeScript + lints and overall code quality has improved.
   
   ----
   
   **History-2:** CI jobs in `apache/(incubator-)superset` took long time to run and was not stable (flaky tests and `master` often had issues at that time). 
   
   **Current:** 🟢  CI and `master` are more stable these days. 🟡 The jobs still take time to run. Probably can mitigate by skipping the unnecessary ones. 
   
   ----
   
   **History-3:** Better tooling and flexibility. We experimented and adopted bots, conventional commits, PR previews (Netlify then later Zeit/Vercel), semi-automatic `npm` packages release via github actions. These are difficult to experiment and configure on repos under `apache` org which we don't have admin rights.
   
   **Current:** 🔴  We still don't have admin rights on `apache/superset` and some apps are not allowed.
   
   ----
   
   **History-4:** There were needs to embedding charts in other apps. Plugins system and publishing the packages on `npm` was for this.
   **Current:** 🟢  Embeddable chart is less of a focus now. 
   
   ----
   
   **History-5:** Some legacy chart plugins were added by community and caused maintenance problems for committers over time. The plugins system and publishing the packages on `npm` hoped to decentralize this a bit. Each org/person can have repo of their custom plugins with repo structure forked from `superset-ui-plugins`. 
   
   **Current:** 🟡  Can still have boilerplate of standalone plugin repo for 3rd party who wants to create plugin, but it will still have issues: keep build config in sync and `npm link`. If the main developers are no longer dogfooding this approach, it is likely to be broken after a while. 
   
   Or we could change the process for creating custom plugins to (1) fork `apache/superset` (2) dev on that and (3) only publish the flagged plugins. However, the forked repo can be heavy and need to keep all parts to be able to rebase from upstream.
   
   -----
   
   So it seems things have changed in favorable direction. 
   
   I agree with the observations @ktmud 's 1-4 above that the workflow has caused new issues with more contributors joining the project. The key questions to be addressed before merging back are also good and I want to add a bit more on top of that.
   
   | Topic | Things to consider before proceeding |
   | ----- | ----- |
   | **Repo tools:** PR preview Storybook via Zeit and Chromatic UI (it's more difficult to request new Github Apps under the apache org, which hosts all other Apache projects, than in apache-superset, which most Superset PMC members have direct admin access). | Do some feasibility study. Check with Apache admin to know what are allowed. Are there workaround and are we OK losing some if not allowed? Better avoid surprises. |
   | **Reduced build time** from smaller codebase and not having to share the GitHub Action runner pool with other Apache projects (Can be mitigated by conditionally triggered workflows) | Make sure we won't save time from `npm link` (fix cost) only to spend more time staring at CI instead (recurring). |
   | **Revise automatic NPM release** | Should be able to set `NPM_SECRET` via JIRA to Apache admins, but have to change publishing flow. Currently, releases are triggered by committers committing a PR that bumps version in `package.json` and CI took care of the rest. Direct `git push` to `master` is not allowed in `apache/superset` |
   | **Figure out `npm` packages release cadence**: How often will the plugins be published? | Currently it is being forced because the main app refer to plugins via package and version in `package.json`. (1) If the `npm` publishing is still a requirement, you still need 2-3 PRs in `apache/superset`. One PR up to the point where plugin is completed. Potentially another PR to bump version and force `npm` publishing. Another PR is to update package version in `superset-frontend` after new packages have been published. (2) The 2-3 PRs process sounds tedious. After moving to the same repo, one might be interested in pointing to plugin `src` directly and bypass the npm publishing at all. This could leave the `npm` packages outdated and someone has to publish at one point, when will that be? This is not a problem for the main app, but 3rd party chart plugin dev may be using outdated `@superset-ui/xxx`. For apps wanting to embed charts, I am less concern on that front due to decreased inter
 est.  |
   | **Maintain quality bar**: 100% test coverage guarantee for core packages (need to update codecov config) | Sounds straightforward. |
   | **3rd party chart plugins**: How would someone develop chart plugins that are only useful for his/her org (without dropping code in `apache/superset`)? | Worth spend sometimes thinking how to provide flexibility while avoid accumulating maintenance cost for `apache/superset`. Refers to the comment above about History-5. |
   
   To sum up, thank you for writing the SIP and thinking more about this problem. I think this could work but want to make sure some homework are taken care of before executing.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org