Skip to content

Conversation

@flyrain
Copy link
Contributor

@flyrain flyrain commented Jun 25, 2025

No description provided.

type: application
version: 0.1.0
appVersion: 1.0.0-incubating-SNAPSHOT
version: 1.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want the Helm chart versioning to always follow the versioning of the underlying app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have to, but we need to update it every time we update the helm-chart itself, see details here, #1944 (comment). I'm OK with other version number, any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm OK with 1.0.0, and I am OK releasing the Helm chart every time we release Polaris, updating version and appVersion.

I am just mentioning this because we never finished the discussion started here:

https://lists.apache.org/thread/rrsjygxdltnbtldyo3zl9x0v0h1t614h

And so I don't know if there is a consensus here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for sharing the thread, that's helpful. I didn't see any objection there. I'm also supportive of release helm charts separately from the main source / binary bundle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And so I don't know if there is a consensus here.

two +1s and no objection for a long time, which is usually considered as a consensus. It'd be nice to conclude it though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for reviving the discussion 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, would you mind chiming in a bit. I hope it can conclude soon. I'm OK if we didn't figure out helm chart release for 1.0.0, but it'd be nice to have it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this discussion to the dev ML?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yufei revived that old thread, and I just added my +1 to his suggestions, FYI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type: application
version: 0.1.0
appVersion: 1.0.0-incubating-SNAPSHOT
version: 1.0.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Helm uses version to track the chart package itself (for repo indexes, upgrades, roll-backs, dependency resolution). It has nothing to do with the container image. It must be valid SemVer. It was recommend to update every time we update the helm chart itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I know that. But why jump from 0.1.0 to 1.0.0 then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason, can you suggest a version?

version: 0.1.0
appVersion: 1.0.0-incubating-SNAPSHOT
version: 1.0.0
appVersion: 1.0.0-incubating
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the docker image tag. The 1.0.0-incubating docker image will be published after the vote passed per release guide suggsted.

Copy link
Contributor

Choose a reason for hiding this comment

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

appVersion has nothing to do with the docker image tag. The docker image tag is set in values.yaml at image.tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

We must keep in mind that a Helm chart published under appVersion X should stay compatible with a wide range of Docker images, roughly going from X-N to X, where N is yet to be defined. At some point, it could be useful to keep a formal compatibility matrix somewhere.

Copy link
Contributor Author

@flyrain flyrain Jun 25, 2025

Choose a reason for hiding this comment

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

Thanks for the correction. In that case, should we update the tag from latest to 1.0.0-incubating in file values.yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We must keep in mind that a Helm chart published under appVersion X should stay compatible with a wide range of Docker images, roughly going from X-N to X, where N is yet to be defined. At some point, it could be useful to keep a formal compatibility matrix somewhere.

How do we archive the the range compatibility? I guess X is 1.0.0-incubating for the Polaris release 1.0.0-incubating. Is there any default value for N?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are pros and cons:

  • latest is very convenient, but at the same time, not very precise.
  • Specifying a fixed version in image.tag, on the contrary, has the merit of making it clear that the Helm chart works best for that specific tag.

I am personally in favor of using image.tag = latest, because if people also install the latest version of the helm chart, and provided that we publish images and charts at every release, then they should automatically get a latest chart version that works well with the latest docker image version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LGTM overall. The only concern is that if users want to revert to a historical version of helm chart, do they expect to still get the latest docker image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is an example, which assumes Helm chart version matched with Polaris src/bin/docker version:

  1. When an user revert to the Helm Chart v1.0.0, it still points the latest docker image, which could be 1.1.0 already. This introduces the incompatible risks. On the other hand, if we specified a version(e.g., 1.0.0) instead of latest, the Helm reversion will always be same.
  2. if people install the latest version of the helm chart, it still points to the latest docker image as we release them together. So that use case is covered as well.

With that, I think specifying a version is preferrable than latest. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a valid point! latest is not very friendly to historical chart versions. I am OK with using the release version instead.

@flyrain
Copy link
Contributor Author

flyrain commented Jun 25, 2025

I will convert this to draft as there are open discussions on helm chart release and tag.

@flyrain flyrain marked this pull request as draft June 25, 2025 20:06
@flyrain flyrain marked this pull request as ready for review June 25, 2025 21:19
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

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

Approving per dev ML discussion

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jun 25, 2025
@flyrain flyrain merged commit 0fab43e into apache:release/1.0.x Jun 25, 2025
2 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jun 25, 2025
@sfc-gh-ygu
Copy link
Contributor

Thanks @adutra and @dimas-b for the review. Will file a similar PR to main soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants