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(helm): add RollingUpdate parameters #20931

Merged

Conversation

gforien
Copy link
Contributor

@gforien gforien commented Aug 1, 2022

SUMMARY

We want to handle the RollingUpdate parameters for the supersetNode and supersetWorker.

How

  • Add the parameters in the deployment.yaml and deployment-worker.yaml templates
  • Get default values from K8S documentation to ensure no change
  • Put default values in values.yaml
  • Update JSON schema
  • Bump chart version

TESTING INSTRUCTIONS

helm lint
helm template . superset-release

ADDITIONAL INFORMATION

  • Has associated issue:
  • 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

@@ -22,7 +22,7 @@ maintainers:
- name: craig-rueda
email: craig@craigrueda.com
url: https://github.com/craig-rueda
version: 0.6.6
version: 0.6.7
Copy link
Member

Choose a reason for hiding this comment

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

This should be 0.7.1

@@ -33,6 +33,11 @@ spec:
matchLabels:
app: {{ template "superset.name" . }}-worker
release: {{ .Release.Name }}
strategy:
type: RollingUpdate
Copy link
Member

Choose a reason for hiding this comment

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

Pls make the strategy configurable. You can just add the whole strategy section into values.yaml and then call toYaml here.

@dpgaspar
Copy link
Member

Hi @gforien would be great if you could rebase and address the requested changes. Thank you!

@gforien
Copy link
Contributor Author

gforien commented Aug 30, 2022

Hi @dpgaspar
Sorry for the delay. I will do this in the upcoming days

@gforien gforien force-pushed the feat/helm/add-rollingupdate-parameters branch from 661b8d1 to f54187c Compare September 21, 2022 08:38
@gforien
Copy link
Contributor Author

gforien commented Sep 21, 2022

Hi 👋
I have rebased my branch, and addressed the requested changes
Thank you for your patience

@gforien
Copy link
Contributor Author

gforien commented Sep 21, 2022

@craig-rueda @dpgaspar

@@ -22,7 +22,7 @@ maintainers:
- name: craig-rueda
email: craig@craigrueda.com
url: https://github.com/craig-rueda
version: 0.7.2
version: 0.7.3
Copy link
Member

Choose a reason for hiding this comment

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

Prob need to bump this one more time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did not know which one you would merge first :)

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.

LGTM

@gforien gforien force-pushed the feat/helm/add-rollingupdate-parameters branch from f54187c to 809696a Compare September 21, 2022 20:53
@craig-rueda craig-rueda merged commit 3f8e9a5 into apache:master Sep 22, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 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/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants