crossplane-contrib / provider-helm

Crossplane Helm Provider
Apache License 2.0
102 stars 65 forks source link

Retry should be enabled by default #193

Open bobh66 opened 1 year ago

bobh66 commented 1 year ago

What happened?

One of Crossplane's best features is that it just keeps retrying when things fail, ever optimistic that something will change in the future to allow things to succeed "next time".

provider-helm is an exception to this - by default it does NOT retry, and there is no way to make it retry forever. rollbackLimit defaults to nil, which disables retry. And there is no way to set rollbackLimit to "forever", such as using 0 or -1. The only option is to set it to a "large" number which will eventually be exceeded.

Ideally the Release would behave like all other Managed Resources and continuously retry until it is successful.

It seems like to be consistent with other providers and Crossplane itself, rollbackLimit should default to a value that specifies infinite retries, or nil should indicate infinite retries and 0 should disable retries.

rollbackLimit is also not an obvious attribute name for an option that controls retries. Maybe a better solution would be to add a retryEnabled attribute that turns retries on and off, and leave rollbackLimit to specify the number of retries, where nil is no limit.

I'd be glad to push a PR if any of these suggestions are agreeable.

How can we reproduce it?

Deploy a Release

turkenh commented 1 year ago

FWIW, provider-helm indeed retries for other errors than a failing release. For example, it would retry forever if it hits an error while pulling the chart or communicating with the Kube API.

I would be supportive of implementing a negative value meaning retry forever, but hesitant to change the default behavior for everyone.

Also related: https://fluxcd.io/flux/components/helm/helmreleases/#configuring-failure-remediation