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: pin version of superset in helm chart #18668

Closed
wants to merge 3 commits into from

Conversation

ad-m
Copy link
Contributor

@ad-m ad-m commented Feb 11, 2022

SUMMARY

I propose to pin version of Superset in Chart.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

There are several reasons for this:

  • during the restart of the pod, no automatic update is performed, which may potentially damage the user's data (backup must be performed before the update, etc.) or affect the availability of the application,
  • new versions of the Superset may require an adaptation of the Superset,
  • old versions of Helm Chart will be known to correctly run old versions of Superset,

The downside is that we need to update the Chart and release it in order for users to use the new versions. Currently, however, releases are automated, and verification of the new Superset release that it is compatible with the Chart is even advisable.

In the future, we can think of an automatic version update when the Superset version is changed. For this, the e2e Superset tests could be useful, perhaps taking into account the update process as well.

TESTING INSTRUCTIONS

I deployed an updated Chart and verified if pods started and inspected created manifests.

ADDITIONAL INFORMATION

  • Has associated issue: Helm installation uses latest tag by default #17540
  • 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

@CarpathianUA ,@quenchua, @alexander-onesoil as affected by #17540, could you take a look and provide feedback?
@craig-rueda as maintainer of Helm Chart, could you take a look?

@CarpathianUA
Copy link

@ad-m
LGTM, but I suggest waiting for the next release and pinning a new version during releasing it.
Example use case:

  1. Yesterday I've updated Superset by executing helm upgrade --install. Since the image version in values.yaml is always latest ( I suppose that the latest tag is corresponds to the last commit in master branch ), I've got Superset image version updated to the latest possible version bound to commit.
  2. After that I've found a 1.4.1 image tag on Docker Hub, and used that tag to update Superset again. After update to 1.4.1 I've got a non-working system - my charts and dashboards got broken ... So if users, who upgraded Superset recently, will update the chart with 1.4.1 most likely they will have issues with Superset after the upgrade. That's why I would prefer to start pinning version of an image with a new release created.

Thanks in advance!

@ad-m
Copy link
Contributor Author

ad-m commented Feb 11, 2022

@CarpathianUA, we had the latest release of Chart 16 hours ago ( https://github.com/apache/superset/releases/tag/superset-helm-chart-0.5.8 ) and we released the latest Superset version before that ( https://github.com/apache/superset/releases/tag/1.4.1 ). Do I understand correctly that you expect no new Chart to be released until Superset >1.4.1. eg. 1.4.2 is released?

@wiktor2200
Copy link
Contributor

wiktor2200 commented Feb 11, 2022

It sounds great. It also caused some problems for me in the past (as far as I remember it was caused by changing container's entrypoint), but then I've just changed image: lastest to selected version, but still having the current stable out-of-the-box would be the best and fail-proof.
I've mentioned it here: #17712 (comment)

@@ -160,7 +160,7 @@ extraConfigMountPath: "/app/configs"

image:
repository: apache/superset
tag: latest
# tag: "v{{ $.Chart.AppVersion }}"
Copy link
Member

Choose a reason for hiding this comment

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

Can we just use this templated value? I think that adding the ability to template these fields would be useful.
i.e.:

tag: "v{{ $.Chart.AppVersion }}"

and then in deployment template, etc:

image: "{{ .Values.image.repository }}:{{ tpl .Values.image.tag . }}"

Choose a reason for hiding this comment

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

BTW, I'm not sure that the referencing "v{{ $.Chart.AppVersion }}" will work at all, since Helm values don't support any templating besides include and template via plane YAML AFAIR

Copy link
Member

Choose a reason for hiding this comment

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

Could be, but it's worth a try :). I've found that in general, the more fields that support templating, the better.

@CarpathianUA
Copy link

@ad-m
I mean that until 1.4.2 I would prefer to keep the default image tag in values as it is (latest tag). When a new release will be published (e.g. 1.4.2) - update the default image tag to 1.4.2 by default. Thanks!

Co-authored-by: wiktor2200 <wiktor2200@users.noreply.github.com>
@stale
Copy link

stale bot commented Apr 18, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 18, 2022
@stale stale bot closed this Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants