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

CSS-6377 add Superset Rock #2

Merged
merged 22 commits into from
Nov 29, 2023
Merged

Conversation

AmberCharitos
Copy link
Contributor

This PR creates a Superset Rock from the apache upstream 2.1.0 tar.
Includes:

  • README for building and running the rock locally with Docker.
  • Addition of startup k8s-init.sh and k8s-bootstrap.sh scripts (previously in charm) in order to enable startup of the Rock independently of the Charm.
  • Superset UI service, to deploy UI directly from Rock.

@AmberCharitos AmberCharitos changed the title CSS-6273 add Superset Rock CSS-6377 add Superset Rock Nov 27, 2023
Copy link

@mertalpt mertalpt left a comment

Choose a reason for hiding this comment

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

Some comments, and also a lot of files do not terminate on a new line. But, looks good overall. No deal breakers, handle the comments as you wish.

.github/workflows/build.yaml Show resolved Hide resolved
.github/workflows/build.yaml Show resolved Hide resolved
.github/workflows/publish.yaml Show resolved Hide resolved
.github/workflows/publish.yaml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
rockcraft.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@maabujayyab maabujayyab left a comment

Choose a reason for hiding this comment

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

Left feedback. We probably should have the Kubeflow team take a look since they built ~100 ROCKs and they'd have some suggestions for us

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
rockcraft.yaml Outdated Show resolved Hide resolved
Copy link

@gtato gtato left a comment

Choose a reason for hiding this comment

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

just a few comments about passwords in config

Copy link

@gtato gtato Nov 28, 2023

Choose a reason for hiding this comment

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

I am a bit confused by the license structure of these rocks. I see Kafka for example has a license "per component", even though sometimes it is difficult to define what a component is... Would you need licences for superset in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From all of the Kubeflow ones I've see it seems the license is per reop - sometimes with multiple rocks as a bundle. I'm not sure what you mean by superset license?

rockcraft.yaml Outdated Show resolved Hide resolved
startup-scripts/k8s-bootstrap.sh Show resolved Hide resolved
startup-scripts/k8s-init.sh Show resolved Hide resolved
startup-scripts/k8s-init.sh Show resolved Hide resolved
Copy link
Contributor

@maabujayyab maabujayyab left a comment

Choose a reason for hiding this comment

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

LGTM

@AmberCharitos AmberCharitos merged commit 0ab09cd into main Nov 29, 2023
2 checks passed
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.

4 participants