feat: set default observedGeneration to -1 on HelmReleases

[makkes]

This resolves an issue with kustomize-controller marking a
Kustomization as healthy even when the helm-controller hasn't even
looked at the HelmRelease targeted by the Kustomization's
healthChecks, yet. Setting observedGeneration to -1 by default will
cause kstatus to report a status of InProgress instead of Ready.

see fluxcd/kustomize-controller#387 for
details on the issues this is solving.

Signed-off-by: Max Jonas Werner [email protected]

[hiddeco]

Would like to study a bit if this is a common pattern, and if there are others. If you have already done this, please tell and I will take it into account.

Given the simplicity however, I am leaning towards simply going with this, but in any case it should be patched on all our controllers.

[makkes]

I absolutely agree that we should investigate how common this way of setting default statuses is. I would suppose a webhook is more common given the flexibility a webhook offers.

in any case it should be patched on all our controllers.

You mean on kustomize-controller, source-controller etc.?

[stefanprodan]

We've discuss webhooks before, I would stay away from them until there is no way around it. Setting defaults for CR fields is not a job for a custom webhook as defaults are fully supported by the Kubernetes builtin admission controller.

[makkes]

I agree that adding a webhook is a pain in the neck. So for now we have discussed these 3 options to fix fluxcd/kustomize-controller#387:

1. Use a webhook. See feat: set default observedGeneration to -1 on HelmReleases #294 (comment). This is painful to implement and adds a lot of overhead for little gain (at least for now).
2. Set a default status.observedGeneration. This is what this PR introduces. It's a very small fix. A drawback is that I haven't found good examples of other projects doing this. It works, though, and fixes the bug.
3. Rely on upstream changes to cli-utils, e.g. to be able to inject custom StatusReaders like I did in this PR. Another upstream change could be for kstatus to actually look at the CRD of a resource and see whether it defines a status field and if it does, mark the resources as Unknown as long as a status hasn't been set.

[hiddeco]

3. Rely on upstream changes to cli-utils, e.g. to be able to inject custom StatusReaders like I did in this PR. Another upstream change could be for kstatus to actually look at the CRD of a resource and see whether it defines a status field and if it does, mark the resources as Unknown as long as a status hasn't been set.

A custom status reader would only solve it for us, but not for others using kstatus with defaults.

[hiddeco]

This looks OK to be merged, but as I am on short leave from Fri - Wed, I am going to wait with merging as it would bump the MINOR version.

This allows allows patch releases to be made without too much hassle during my absence.

[hiddeco]

@makkes can you please rebase instead of merging in main? I promise to merge right after 🤞

[hiddeco]

Thank you @makkes, and sorry for the wait 🍻