search

operator-framework / helm-operator-plugins

Experimental refactoring of the operator-framework's helm operator
Apache License 2.0
47 stars 47 forks source link

Apply updates from operator-sdk helm-operator #63

Open joelanford opened 3 years ago

joelanford commented 3 years ago
anmol372 commented 3 years ago

~PR for Build info metric: 67~ PR to add correct help text: kb-1830 ~PR for Plugin docker invocation: 68~

mikeshng commented 3 years ago

FYI There is also this https://github.com/operator-framework/operator-sdk/pull/4288 that was merged

mikeshng commented 3 years ago

If possible, could you please also consider https://github.com/operator-framework/operator-sdk/pull/4297 Due to the unintended uninstall rollback, some of our customers were hit with unintended data loss.

joelanford commented 3 years ago

Thanks @mikeshng if there are any other Helm-related PRs that you're aware of, please post them.

We're fairly confident that this repo has everything up to SDK v1.0.0.

Our goal is to eventually replace the SDK helm-operator implementation with this one, and we want to make sure all the bug fixes and added features from the SDK repo make it here.

mikeshng commented 3 years ago

Thanks @joelanford for all your help. Two PRs that I am less confident about and I am looking for your team's expert opinions on:

joelanford commented 3 years ago

For operator-framework/operator-sdk#4288, is this just a defensive check, or is there actually a code path that results in a nil error and a nil UninstallReleaseResponse or nil Release? I'm not seeing a way for the existing Helm libraries to return both a nil Release and a nil error.

joelanford commented 3 years ago

I don't think operator-framework/operator-sdk#4358 is applicable here. In this repo, there is nothing that deletes releases from the storage backend outside of running through an uninstall. manager.Sync has been mostly replaced by getReleaseState.

When a non-deployed release exists, it looks like this repo will attempt an upgrade rather than pre-deleting it and then attempting a clean install. If the last release is one of Deployed, Failed, or Superseded, then the upgrade will proceed. That seems reasonable in the context of the Helm operator too I think. I'll submit a PR here to make sure we attempt the upgrade in this scenario.

mikeshng commented 3 years ago

For operator-framework/operator-sdk#4288, is this just a defensive check, or is there actually a code path that results in a nil error and a nil UninstallReleaseResponse or nil Release? I'm not seeing a way for the existing Helm libraries to return both a nil Release and a nil error.

As I remember, I ran into a nil pointer in the old code. It was only doing:

uninstallResponse, err := uninstall.Run(m.releaseName)
return uninstallResponse.Release, err

So the primary goal of the fix was to avoid this nilpointer when uninstallResponse is nil.

As for the change if log.V(0).Enabled() && uninstalledRelease != nil { Yes, I think that's a defensive check that Eric suggested.

joelanford commented 3 years ago

Re: operator-framework/operator-sdk#4288 - It looks like this is also not applicable in this repo since the client returns the uninstall response and then the caller correctly checks for errors before assuming the embedded release field is valid.

mikeshng commented 3 years ago

@joelanford if you have some cycles, could you please see my idea here and post a feedback message: https://lists.cncf.io/g/cncf-helm/message/351 I am trying to enhance the helm uninstall so the helm operator can leverage the new functionality and perform a more reliable resource cleanup upon CR delete. Thanks.

jmrodri commented 2 years ago

@varshaprasad96 please review to see where we are with this, close when done.

varshaprasad96 commented 2 years ago

Went through the updates with respect to helm-operator in operator-sdk after SDK 1.0 release till v1.15.0. These are the following updates:

PS: These are updates made to the library, the plugin was updated recently and is already upto date.