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

feat: publish superset helm chart #14163

Merged
merged 8 commits into from
Apr 16, 2021
Merged

feat: publish superset helm chart #14163

merged 8 commits into from
Apr 16, 2021

Conversation

jawabuu
Copy link
Contributor

@jawabuu jawabuu commented Apr 15, 2021

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
Member

Thank you so much for your contribution!!

Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

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

Thanks, this is great!

Copy link
Member

@craig-rueda craig-rueda left a comment

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

.github/workflows/superset-helm-release.yml Outdated Show resolved Hide resolved
.github/workflows/superset-helm-release.yml Show resolved Hide resolved
@danielewood
Copy link
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
Copy link

Hokwang commented Apr 16, 2021

waiting this

@jawabuu
Copy link
Contributor Author

jawabuu commented Apr 16, 2021

@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
Member

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

@jawabuu
Copy link
Contributor Author

jawabuu commented Apr 16, 2021

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.

Copy link
Member

@craig-rueda craig-rueda left a comment

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
Copy link
Contributor Author

jawabuu commented Apr 16, 2021

@craig-rueda on standby : )

@craig-rueda
Copy link
Member

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

@jawabuu
Copy link
Contributor Author

jawabuu commented Apr 16, 2021

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

@craig-rueda
Copy link
Member

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

@jawabuu
Copy link
Contributor Author

jawabuu commented Apr 16, 2021

Thank you!

@jawabuu
Copy link
Contributor Author

jawabuu commented Apr 16, 2021

@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 labels Mar 12, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants