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

build(helm): Helm-docs + fix Helm chart release #21963

Merged
merged 13 commits into from
Oct 29, 2022

Conversation

Yann-J
Copy link
Contributor

@Yann-J Yann-J commented Oct 28, 2022

SUMMARY

This PR implements 2 improvements to the releasing of Helm chart:

  • Support for running helm-docs to generate a README for the chart, automatically generated from the Chart.yaml and annotated values.yaml. It will be enforced both as a pre-commit check, as well as a CI check.
  • Fix the helm chart-releaser action which seems broken due to missing dependency on the bitnami chart repo (which prevented the release of the updated chart from feat(helm): Support for flower and websocket containers #21806 ) - this may need some trial & error to confirm a successful release.

This PR includes a (useless) bump of the chart version to force the CI to go through a Helm release.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

See generated Chart README:

image

TESTING INSTRUCTIONS

  • For the helm-docs pre-commit:
    • Check out project
    • Update pre-commit (pre-commit install)
    • Make some chart update (e.g. bump up the version in Chart.yaml) and stage the change
    • Try to commit. Git should raise an error complaining that a pre-commit check has failed
    • Run pre-commit run helm-docs to force execute the helm-docs generation, stage the resulting change and commit again
  • For the chart releaser fix: merge the PR and hope for helm chart v0.7.6 to be released!

ADDITIONAL INFORMATION

  • Has associated issue: feat(helm): Support for flower and websocket containers #21806 (merge failed to release)
  • Required feature flags:
  • 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

@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Merging #21963 (de9f2bf) into master (102909e) will increase coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #21963      +/-   ##
==========================================
+ Coverage   66.86%   66.96%   +0.09%     
==========================================
  Files        1807     1807              
  Lines       69218    69215       -3     
  Branches     7402     7402              
==========================================
+ Hits        46280    46347      +67     
+ Misses      21033    20963      -70     
  Partials     1905     1905              
Flag Coverage Δ
hive 52.92% <ø> (?)
mysql 78.36% <ø> (+<0.01%) ⬆️
postgres 78.43% <ø> (+<0.01%) ⬆️
presto 52.82% <ø> (+<0.01%) ⬆️
python 81.49% <ø> (+0.20%) ⬆️
sqlite 76.91% <ø> (+<0.01%) ⬆️
unit 51.09% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/utils/core.py 90.31% <0.00%> (+0.09%) ⬆️
superset/connectors/sqla/models.py 91.01% <0.00%> (+0.17%) ⬆️
superset/reports/commands/execute.py 92.28% <0.00%> (+0.29%) ⬆️
superset/db_engine_specs/base.py 89.69% <0.00%> (+0.31%) ⬆️
superset/db_engine_specs/hive.py 87.35% <0.00%> (+16.20%) ⬆️
superset/db_engines/hive.py 85.18% <0.00%> (+85.18%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Yann-J
Copy link
Contributor Author

Yann-J commented Oct 28, 2022

Hmm, struggling a bit to set up helm-docs in CI, because of actions/runner-images#6283

Should be OK now with latest commit, let's see when CI completes...

@Yann-J
Copy link
Contributor Author

Yann-J commented Oct 29, 2022

Alright @craig-rueda after much tweaking I have the CI now fairly clean!

I finally opted not to run a separate Github action to check for helm-docs, since I see that CI will anyway run all the pre-commit checks, so these alone are enough. It was slightly harder than it should be, because of the above mentioned issue that makes using brew a bit harder on Github machines.

There is still a CI failure with one of the cypress e2e tests, but I can see it's a known flaky test, and is very unlikely to be related to the changes in this PR.

Note that since this would (hopefully) be the first successful helm chart release since #20722 this should also force the metadata refresh in artifacthub, and enable the 'verified publisher' badge.

@craig-rueda
Copy link
Member

Awesome, thanks for this! Ready to merge?

@Yann-J
Copy link
Contributor Author

Yann-J commented Oct 29, 2022

Yes I think we're good!

@craig-rueda craig-rueda merged commit ae9a30b into apache:master Oct 29, 2022
@Yann-J
Copy link
Contributor Author

Yann-J commented Oct 30, 2022

Perfect, chart succesfully released, and the artifacthub page now has icon, docs, and Verified badge!

image

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 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/M 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants