Do not update helm_release state if chart download/parse error

[mbarrien]

Description

Do not update helm_release state if chart fails to download (server down, bad chart path, bad chart version), or chart fails to parse/load.

This is achieved by wrapping the chart downloading and parsing inside a partial resource state, inside of which we don't actually set any fields as partially complete. This prevents saving bad chart state to the state file. Most importantly, it does not update any changed fields such as "chart" or "repository" if we fail during these steps, which is valid since during these steps no state has actually been changed on the Kubernetes cluster.

I'm not convinced this is the "right" solution (since most other resources don't have to resort to partial state updates), but it does work in my uses.

Acceptance tests

• [ ] Have you added an acceptance test for the functionality being added?
• [ ] Have you run the acceptance tests on this branch? (If so, please include the test log in a gist)

References

Fixes #472 (or at least part of it). Fixes regression introduced in #449.

[mcuadros]

How this improved the feature introduced by https://github.com/terraform-providers/terraform-provider-helm/pull/464, any parse error or download error should be detected on then plan phase, so what is described here should happen.

[mbarrien]

Fact of the matter is that it's NOT being caught in plan phase, otherwise I wouldn't have written this. See #472 for how to reproduce. Verified a problem in TF Helm provider release 1.2.1 (the latest)

[mcuadros]

Sorry, I am lost your text claims that this handles the problem of server down, bad chart path, bad chart version, but in fact, is solve a problem with the repositories? That's why I asked because this should be covered by #464.

So will be great if you write the acceptance tests.

Thanks for the PR.

[mbarrien]

Did some more investigation into @mcuadros assertion that the #464 should be enough to catch the problem in the plan phase. I found that the error I'm catching here is being thrown then ignored before it reaches any of the code in that PR. The call to getChart() at https://github.com/terraform-providers/terraform-provider-helm/blob/v1.2.1/helm/resource_release.go#L641 is properly returning an error for my case described in #472 chart "foo/metrics-server" version "2.11.1" not found in https://kubernetes-charts.storage.googleapis.com repository, but 2 lines down in https://github.com/terraform-providers/terraform-provider-helm/blob/v1.2.1/helm/resource_release.go#L643 the error is ignored and nil is returned.

I don't know what the right behavior should be, but this PR may not be the right answer for the bug.

[mbarrien]

Please see #516 for alternate fix. I need feedback to know which approach is the better one, given that #516 breaks some tests but in deprecated code. The underlying bug is cause by a "feature" introduced in an earlier PR.

In either case, this is a proven issue that causes the Terraform state file to be in an inconsistent state and is unfixable unless you know the underlying bug; I'm preventing rolling out of Terraform Helm provider at my organization until this issue is resolved.

[ksvladimir]

Hi @mbarrien and @mcuadros , I hit this same problem in my CI infrastructure, and I can confirm that this patch fixes it. It would be great to see it merged!

[k0t3n]

Any updates?

[hashicorp-cla]

All committers have signed the CLA.