cert-manager: fast-forward to upstream 36289cf0

[munnerz]

• Automated cherry pick of incubator/mongodb-replicaset bootstrap init-container has a possible infinite loop #522 (Automated cherry pick of #522 cert-manager/cert-manager#615)

[timstoop]

As this change adds a feature, I'd recommend bumping the minor version.

[munnerz]

Has there been some consensus on what is recommended here? In the past this has not been a requirement, and the majority of charts have used the minor version to indicate breaking changes.

For what it's worth, I am inclined to prefer to close this PR and revert the change in our release-0.3 branch rather than bump the minor version here.

I have previously asked about versioning applications vs charts in the past to get clarification: helm/helm#3555

We specifically choose to keep the minor version of our chart aligned with the minor application version (although patch version numbers can, and do drift apart).

This is because we may add a new flag to a new release of cert-manager, and by adding support for it into the Helm chart would break compatibility with older releases of cert-manager, as they do not support the flag. This is a pretty fundamental question around Helm chart versioning in general IMO, and one we don't have a good story for.

I know it isn't recommended to try and align appVersion with chart version, but this way, a user knows they can use Helm chart version 0.3.x with any of cert-manager 0.3.x, and chart version 0.4.x with cert-manager 0.4.x. Compatibility is defined within the minor version.

Keen to hear thoughts on how this should best be handled. I think allowing users to be able to easily understand the compatibility boundaries of a chart is very important, so as I say, I'd sooner just not add any new features to this chart until we release cert-manager v0.4, than lose this property just now (but I know a lot of users would like to be able to use new chart features before the new release of cert-manager is ready, hence why we cherry pick backwards compatible feature additions into minor releases of the Helm chart! 😄)

/cc @unguiculus

[timstoop]

I believe we follow the guidelines from https://semver.org/, which indicate: Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. (https://semver.org/#spec-item-7)

I understand your reasoning regarding app version, but I think that a specific version of a chart is meant for a specific version of an application, as noted with the appVersion key in the Chart.yaml. As far as I know, most other charts do not tie their Chart version to specific app version ranges, but if this is important to you, it may make more sense to simply have a table in the README instead, as that's the first documentation a user gets to see. I personally appreciate the table provided by the cluster-autoscaler (https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler#releases).

[timstoop]

(That said, we don't seem to follow it to the letter, as most Charts are still at major version 0. But still, I think it's a good guideline to keep in mind.)

[munnerz]

I believe we follow the guidelines from https://semver.org/, which indicate: Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. (https://semver.org/#spec-item-7)

Agreed - however historically next to no charts in this repo have been following semver, so I wonder if there has been an actual policy change for k/charts or not 😄

I understand your reasoning regarding app version, but I think that a specific version of a chart is meant for a specific version of an application

This would be nice, although would imply it is not possible to use e.g. (for ex) the new 'taints' feature added to the Helm chart, with an older release of the application (which is not a reflection of reality).

I personally appreciate the table provided by the cluster-autoscaler

The table provided in the cluster-autoscaler chart is specifically mapping Kubernetes version->Cluster Autoscaler version, which is a subtly different issue (and one we also experience with cert-manager, although we are far more lax on our versioning against kubernetes and are able to support many K8s releases with a single cert-manager release).

The issue here is around versioning the chart against the application. Maintaining a table as such seems pretty tedious and less helpful for users, as they will always have to refer to the README whenever they want to install cert-manager. This issue would be alleviated if we had some way to helm install a specific appVersion (instead of chart version, as proposed in helm/helm#3555), however it is still not ideal.

Thanks for engaging in this discussion 😄 it's a wider issue with Helm and chart versioning imo, and it's one I'd like to come to a definitive conclusion on the best practice for, especially taking into account UX for end-users, who may not understand the intricacies/differences between an application version and a chart version (and instead view the two as one and the same).

[timstoop]

I've been pushing for following the semver guidelines in the last months without much push back, so I suspect it's more or less accepted ;-) That said, having a proper guideline about it would indeed be best, one way or the other.

Also, I think that appVersion is actually a string, so you might get away with simply having a range of versions in there?

Regarding my example, yes, it's subtly different, but the same solution could be used for this problem as well. I don't think updating the table in the README is that much more tedious than updating any other part of the documentation after a change.

I'm not necessarily against it, mind you, I just prefer to follow a clear and defined standard in this regard. I'm hoping that the helm client will get support for warnings based on semver versions, so a user can make a more informed decision with regards to updates. But I have no idea if such a mechanism is even considered at this time.

[munnerz]

I've been pushing for following the semver guidelines in the last months without much push back, so I suspect it's more or less accepted ;-)

Great to hear there is some action being taken here, but I think this needs more careful consideration in order to handle the cases I've raised in this thread before it is made ubiquitous.

There needs to be some mechanism in Helm to handle version skew/compatibility, as the way an application is deployed is intrinsically linked to its version, and it is not acceptable to simply say that we should only maintain/support the Helm chart for the HEAD of the repo.

I think have a proper guideline here isn't just best, but is really essential if we're to make these kinds of decisions. Not addressing it simply shifts the burden onto end-users, who are often some of the least well equipped as they are often both new to Helm, Kubernetes and cert-manager 😄

Regarding having the README maintain a compatibility matrix - this falls down when you start cherry picking changes into different releases, as there is no support for multiple release 'channels' or the like in Helm, and we may end out with 0.5.1 on the release-0.2 branch being different to 0.5.1 on the release-0.3 branch.

So far I've yet to find any reliable way to support a chart for both v0.2 of my app as well as v0.3, in a way that allows me to backport critical fixes for the v0.2 release whilst releasing new features for v0.3.

[munnerz]

Hey, any updates here?

I'm happy to follow the upstream guidance, however I'd like to see that guidance make its way to the 'helm best practices' website beforehand, as I am reluctant to break/change our current versioning pattern unless it is absolutely decided on how versioning should be handled, as this will be an irreversible change which does not 'solve' any of the issues users have complained to me about regarding how we version our chart 😄.

Is there some other issue/thread where this is being discussed? As it stands, I cannot push any updates to our Helm chart until this issue is resolved 😄

[unguiculus]

Merging it. Let's continue the discussion elsewhere.

[unguiculus]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz, unguiculus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• stable/cert-manager/OWNERS [munnerz,unguiculus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment