search

nextcloud / helm

A community maintained helm chart for deploying Nextcloud on Kubernetes.
GNU Affero General Public License v3.0
295 stars 258 forks source link

feat: Update appVersion to 29.0.0 #564

Closed provokateurin closed 2 months ago

provokateurin commented 2 months ago

Pull Request

Latest version.

Checklist

tamcore commented 2 months ago

A major appVersion update as a patch bump for the chart is IMHO not the greatest idea. It's too easy to slip through and break stuff for the end user.

provokateurin commented 2 months ago

We had this discussions a few times already and my take is the following: The desired version can be configured by the user in the values.yaml and the appVersion is only a default value. The chart works with any Nextcloud version, so this is not a breaking change. The same reasoning is usually applied for libraries with semver and their dependencies. If you update a dependency of that library it is only a patch bump regardless of the bump type the dependency had.

twistedgrim commented 2 months ago

Gotta love breaking changes with minor version bumps. Tried to roll back version after the new minor version bump.. 🙃 

Can't start Nextcloud because the version of the data (29.0.0.19) is higher than the docker image version (28.0.4.1) and downgrading is not supported. Are you sure you have pulled the newest image version?
provokateurin commented 2 months ago

Hey, please stay friendly. I see that people are frustrated by this. Do you think simply bumping the major version of the chart as well will fix it?

twistedgrim commented 2 months ago

Not trying to say anything unfriendly. Thank you for the work, and I understand this is community maintained. It just feels like we might need some guidelines on what constitutes the various semver changes within the helm chart. Is it the helm chart itself or the app releases. IMO if there is a major change within the app it should constitute at least a minor change within the chart so one could investigate further if they wish. Again, I totally understand this is community driven and there is nothing governing the versioning requirements of the helm chart here.

provokateurin commented 2 months ago

I think the alternative would be to drop the appVersion completely and have everyone explicitly configure the docker image version they want. That way no new release is needed for changing the default version which I'd see as a benefit because this has to happen every release right now.

jessebot commented 1 month ago

Late the party, but I think @tamcore is correct, as this is standard for all helm charts. I should have brought it up before approving (sorry about that 🤦 ), but they are right, major version of a helm chart application should be a bump to the helm chart major version, or at least the minor version. A patch is meant for patch versions or simple fixes that can't break anything else. A major nextcloud version is significant, as users cannot roll back major nextcloud versions easily.

We had this discussions a few times already and my take is the following: The desired version can be configured by the user in the values.yaml and the appVersion is only a default value.

We have indeed had this conversation a few times, and if a user wants to, they can already specify the tag explicitly, so they can avoid this issue entirely, but the appVersion should still be specified for the main chart, as we need it for testing the latest version of nextcloud with the latest version of the helm chart templates. To see an example of the current helm standard, do a helm create test-chart. You can also look at other popular rapidly maintained helm charts such as ArgoCD. (here is an example where a minor verison because the appVersion was updated a minior version)

I think the alternative would be to drop the appVersion completely and have everyone explicitly configure the docker image version they want.

That is non-standard in helm charts. We should instead have a system such as renovateBot that automatically bumps both the appVersion and the chart version to the same semver patch section. Please do not remove the default appVersion. Please instead double check you are bumping the correct part of the helm version. We all make mistakes, myself often included, and that's totally fine. I'm not upset at all btw (just noting that because text is hard to read tone in 💙 ). I just want to make sure we follow standards for semver on this one: https://semver.org/