Skip to content

feat: publish superset helm chart#14163

Merged
craig-rueda merged 8 commits into
apache:masterfrom
jawabuu:publish-helm-chart
Apr 16, 2021
Merged

feat: publish superset helm chart#14163
craig-rueda merged 8 commits into
apache:masterfrom
jawabuu:publish-helm-chart

Conversation

@jawabuu

@jawabuu jawabuu commented Apr 15, 2021

Copy link
Copy Markdown
Contributor

SUMMARY

This PR adds a simple chart releaser action to package and publish the superset helm chart.
A chart release will be created with the name superset-helm-chart-{{version}}
The chart repo will be available at https://apache.github.io/superset
A user will only need to run the following to install superset as opposed to cloning the entire repository.
helm repo add superset https://apache.github.io/superset
helm install superset/superset

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Clone the repo.

ADDITIONAL INFORMATION

  • Has associated issue: Package helm chart #14037
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API
    @amitmiran137

@amitmiran137

Copy link
Copy Markdown
Member

Thank you so much for your contribution!!

@craig-rueda craig-rueda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, this is great!

@craig-rueda craig-rueda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

After taking a closer look, I think we need a few tweaks

Comment thread .github/workflows/superset-helm-release.yml Outdated
Comment thread .github/workflows/superset-helm-release.yml
@danielewood

Copy link
Copy Markdown
Contributor

Has this been tested? I was going to be submitting a nearly identical PR. I ran into some issues when creating my action. Namely I needed to add the upstream chart repos to the released action. You can see what I did and copy that over if you want:
https://github.com/danielewood/charts/blob/f38d51ffc34e46fe0e58d561878fed3e1ac326b1/.github/workflows/release-charts.yaml#L24-L26

Also, is it the intent to publish a new chart on every PR merge? I would assume you only publish a new chart of you have modified the existing charts.

You can add:

paths:
    - 'helm/**'

To make this action trigger on only modifications to the charts.

@pull-request-size pull-request-size Bot added size/L and removed size/M labels Apr 15, 2021
@Hokwang

Hokwang commented Apr 16, 2021

Copy link
Copy Markdown

waiting this

@jawabuu

jawabuu commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

@craig-rueda Should be good now.
Lint is running on pull requests and will fail if chart changes were made but version was not incremented.
Release will package and create artifact only on push to master. Note that artifact creation will fail if there is an existing release with the same tag but this will already be caught by the linter.

@jawabuu
jawabuu requested a review from craig-rueda April 16, 2021 11:10
@craig-rueda

Copy link
Copy Markdown
Member

Can you also make a dummy change to something in the chart so we can see the linter working?

@jawabuu

jawabuu commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

Can you also make a dummy change to something in the chart so we can see the linter working?

Done. However I don't think new tests in a PR are run, they need to be in master if I'm not wrong.

@craig-rueda craig-rueda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fair enough. Let's try it.

@craig-rueda
craig-rueda merged commit c3e89d5 into apache:master Apr 16, 2021
@jawabuu

jawabuu commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

@craig-rueda on standby : )

@craig-rueda

Copy link
Copy Markdown
Member

Looks like it failed: https://github.com/apache/superset/actions/runs/756228516

@jawabuu

jawabuu commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

Yes it did. Seems you have policies on what actions can run in your repo. Let me check.

@craig-rueda

Copy link
Copy Markdown
Member

I'll open a ticket with Apache Infra and see if I can get it allow-listed

@jawabuu

jawabuu commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

Thank you!

@jawabuu

jawabuu commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

@craig-rueda Once you do, we'll probably need to update the documentation to point users to https://apache.github.io/superset as the superset helm repo.

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* Create initial helm-release action for superset

* Add lintconf to ensure chart passes validation

* Add lint-test job

* Add apache licence headers

* Run job for master only

* Move helm-lint to separate workflow

* Helm release for master & helm dir changes only

* Dummy change to test linter
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.0 labels Mar 12, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
* Create initial helm-release action for superset

* Add lintconf to ensure chart passes validation

* Add lint-test job

* Add apache licence headers

* Run job for master only

* Move helm-lint to separate workflow

* Helm release for master & helm dir changes only

* Dummy change to test linter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.2.0 First shipped in 1.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants