terraform-provider-helm icon indicating copy to clipboard operation
terraform-provider-helm copied to clipboard

Enable "helm upgrade --install" equivalent behavior

Open n-oden opened this issue 1 year ago • 21 comments

Description

This PR adds a (potentially) idempotent "upgrade mode" to the provider, mimicking the behavior of helm upgrade --install as defined in https://github.com/helm/helm/blob/main/cmd/helm/upgrade.go

To wit: an upgrade block is added to the resource attributes, consisting of two boolean values: enable and install.

If enabled is true, this causes the provider to create a *action.Upgrade client, and attempts to perform an upgrade on the named chart. If install is true, it will first create an *action.History client to determine if a release already exists; if it does not find one it creates an *action.Install client and attempts to install the release from scratch. If a release is found, an upgrade is performed. This allows the resource to potentially co-exist with, e.g., CI/CD systems that could potentially install the release out-of-band from terraform's viewpoint.

Acceptance tests

  • [X] Have you added an acceptance test for the functionality being added?

Release Note

Release note for CHANGELOG:

ENHANCEMENT:
* `resource/helm_release`: add `upgrade` map attribute to enable idempotent release installation, addressing components of [GH-425](https://github.com/hashicorp/terraform-provider-helm/issues/425)

References

  • https://github.com/hashicorp/terraform-provider-helm/issues/425

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

n-oden avatar Sep 07 '23 16:09 n-oden

CLA assistant check
All committers have signed the CLA.

hashicorp-cla avatar Sep 07 '23 16:09 hashicorp-cla

As I noted over in the #425, I've released a test build of this for people to kick the tires on:

https://registry.terraform.io/providers/n-oden/helm/latest

n-oden avatar Sep 17 '23 15:09 n-oden

Bump: could someone re-approve the checks? I've updated the PR to address the failing ones.

n-oden avatar Sep 28 '23 13:09 n-oden

I tried to build and use it locally, the terraform validation fails with

│ Error: failed to read schema for module.deploy_promtail.helm_release.this in local.dev/testing/helm: provider local.dev/testing/helm has invalid schema for managed resource type "helm_release", which is a bug in the provider: "1 error occurred:
	* upgrade.install: must set Optional, Required or Computed

(Terraform v1.6.0 on linux_amd64)

bd-spl avatar Oct 18 '23 08:10 bd-spl

Updated, all tests passing locally.

n-oden avatar Oct 24 '23 14:10 n-oden

@bd-spl gentle nudge? :)

n-oden avatar Oct 31 '23 13:10 n-oden

@bd-spl gentle nudge? :)

I welcome this change, albeit have no merging powers here. Please try reaching out the owners of this repository. I think the 'copywrite' check is blocking this effort?

bd-spl avatar Nov 08 '23 09:11 bd-spl

@alexsomesan can I prevail on you to take a look here? I think this is ready to go.

n-oden avatar Nov 09 '23 19:11 n-oden

@alexsomesan nudge? :)

n-oden avatar Nov 29 '23 14:11 n-oden

@alexsomesan this is now rebased on top of the current main branch. Is there any chance this could be reviewed before EOY? I've got an internal project that's rather blocked by this and would like to avoid the temptation to use a forked version without a formal 👍 on the approach from your team.

n-oden avatar Dec 13 '23 16:12 n-oden

Happy New Year everyone! @alexsomesan my apologies for being a bother, but would it be possible to get the test workflows approved? The tests are passing locally and I believe absent any other feedback that this is ready for submission.

n-oden avatar Jan 03 '24 15:01 n-oden

Is there any news on this?

lodmfjord avatar Jan 25 '24 10:01 lodmfjord

@lodmfjord I'm honestly not sure what's going on here; we seem to have fallen into a bit of a hole. In the meantime if you'd like to kick the tires on this you can use https://registry.terraform.io/providers/n-oden/helm/0.0.2 which is a fork of the hashicorp helm provider with this PR cherry-picked on top. Obviously the caveat there is that there's no guarantee that they'll ever merge this back upstream, or hashicorp might ask for incompatible state schema changes before they do, so using it in production is very much an at your own risk proposition and I probably can't personally commit to keeping a fork up to date in perpetuity if this PR never gets accepted.

n-oden avatar Jan 26 '24 14:01 n-oden

@arybolovlev @SarahFrench @appilon @jrhouston my apologies for the spam, but I'm hoping to get someone to approve running the checks on this PR, which seems to have gotten a little lost.

n-oden avatar Feb 20 '24 15:02 n-oden

We're having a look at this, but I personally have to familiarise myself with the helm upgrade mechanism itself before I can evaluate this PR. I'm doing that, in the background of other tasks.

alexsomesan avatar Apr 05 '24 13:04 alexsomesan

@alexsomesan many thanks, and if I can answer any questions please don't hesitate to ask

n-oden avatar Apr 06 '24 16:04 n-oden

@alexsomesan just checking in: I've rebased on the current upstream main branch and dealt with the merge conflicts. Could you approve the testing workflows?

n-oden avatar Apr 23 '24 18:04 n-oden

@alexsomesan thanks! I'm a little baffled as to why the 0.12 test is failing, and the output gives no hint. Can you re-run that with debug logging enabled?

n-oden avatar Apr 24 '24 16:04 n-oden

FWIW I cobbled together a local test environment and was unable to reproduce the test failure using tf v0.12.31:

https://gist.github.com/n-oden/99afa11625eee21c49efddf6ebc896db

(It did eventually fail, down in TestAccResourceRelease_delete_regression but I suspect this is due to my jerry-rigged test container and not any actual code issues.)

n-oden avatar Apr 24 '24 17:04 n-oden

After some more experimentationI'm gonna strongly assert that whatever was going on with the v0.12.31 test in the previous run was not a result of this PR. I set up a parallel PR in my own repo to make the tests run there and you can see here that it's passing: https://github.com/odenio/terraform-provider-helm/pull/1

@alexsomesan I've rebased this against the current main/HEAD -- can you re-approve the test runs?

n-oden avatar May 02 '24 14:05 n-oden

@BBBmau welcome aboard. I've rebased on top of the current main branch and updated to the new documentation strategy. Feel free to ping me on the kubernetes slack if you have questions.

n-oden avatar Jun 28 '24 15:06 n-oden

@BBBmau the failure in https://github.com/hashicorp/terraform-provider-helm/actions/runs/9781860920/job/27286412402?pr=1247 doesn't seem like it's related to my change? But if you think otherwise let me know and I'll go hunting.

n-oden avatar Jul 10 '24 19:07 n-oden

@BBBmau the failure in https://github.com/hashicorp/terraform-provider-helm/actions/runs/9781860920/job/27286412402?pr=1247 doesn't seem like it's related to my change? But if you think otherwise let me know and I'll go hunting.

Yeah the failure isn't really related to your changes. I'll be looking into this in order to get this merged ASAP.

BBBmau avatar Jul 10 '24 20:07 BBBmau

Some things I've come across while reviewing this:

Used bitnami/redis chart for testing purposes.

I attempted to do a clean install with upgrade.enable = true I ran into the error which informs me that I need to either:

  1. perform the install without the upgrade block or
  2. perform the install with upgrade block by also setting upgrade.install = true

This makes sense and follows the documentation written in this PR, I went ahead and performed an install without the upgrade block. I proceeded to include upgrade.install = true right after with no issues.

I also attempted the following which matches the helm upgrade <RELEASE> <CHART> which upgrades the chart to the latest version

  1. installed bitnami/redis with no version set, marking the version as the latest (19.6.1)
  2. explicitly add version = 19.6.0, changing the version 19.6.1 -> 19.6.0
  3. remove version to see whether the version is upgrade to 19.6.1This results in no changes outputted by terraform

This is good news since this PR should only support helm upgrade --install, auto upgarding to the latest release is not something we want to support in the provider. This is noted here

I then attempted the case that users run into which is

│ Error: cannot re-use a name that is still in use
│ 
│   with helm_release.upgrade,
│   on main.tf line 1, in resource "helm_release" "upgrade":
│    1: resource "helm_release" "upgrade" {
│ 
╵

adding upgade.enable = true resolves it

i've also tested postrender block with upgrade by using one of the post-renderer demos: https://github.com/thomastaylor312/advanced-helm-demos/tree/master/post-render

resource "helm_release" "upgrade" {
  chart = "bitnami/redis"
  name = "upgrade-test"
#   version = "19.6.1"
  namespace = "mau"
  upgrade {
    enable = true
  }
  postrender {
    binary_path = "./kustomize"
  }
}

Looks to also be working the same as regular helm install --post-render

This PR looks to be in good standing, apart from the comment on the test the only other things is checking the docs. Any feedback on that? @jrhouston

BBBmau avatar Jul 12 '24 19:07 BBBmau

This is also the reason for Deferred Actions / acceptance_tests workflow not passing on your PR: https://github.com/orgs/community/discussions/44322

BBBmau avatar Jul 15 '24 21:07 BBBmau

@BBBmau oy gevalt. Well, that's definitely a bit out of scope for this PR to fix. :)

n-oden avatar Jul 15 '24 21:07 n-oden

@n-oden recent commits should have fixed the Deferred Actions workflow from failing on PRs coming from forks. Can you rebase to test it out?

BBBmau avatar Jul 25 '24 19:07 BBBmau

@BBBmau done! I'll need you to approve running the workflows.

n-oden avatar Jul 26 '24 13:07 n-oden

@BBBmau The failure in the docs build is not me:

/home/runner/work/_temp/744b2ab9-9524-42d0-a52a-c19b314fdf87.sh: line 1: Making: command not found

The problem here is this line:

e5aa8edbbc (Sheneska Williams 2024-07-23 11:21:48 -0400 40)             echo "Documentation is not up to date. Please refer to the `Making Changes` in the Contribution Guide on how to properly update documentation."

Because the string being fed to echo there is double-quoted, the backticks around "Making Changes" are being treated as a shell escape. cc @sheneska

I'm updating this PR to fix that, but this should probably not have made it into the main branch in the first place.

n-oden avatar Jul 31 '24 14:07 n-oden

@BBBmau The failure in the docs build is not me:

/home/runner/work/_temp/744b2ab9-9524-42d0-a52a-c19b314fdf87.sh: line 1: Making: command not found

The problem here is this line:

e5aa8edbbc (Sheneska Williams 2024-07-23 11:21:48 -0400 40)             echo "Documentation is not up to date. Please refer to the `Making Changes` in the Contribution Guide on how to properly update documentation."

Because the string being fed to echo there is double-quoted, the backticks around "Making Changes" are being treated as a shell escape. cc @sheneska

I'm updating this PR to fix that, but this should probably not have made it into the main branch in the first place.

thanks for looking into that but ideally we'd want the fix to be in a separate PR. can you include the fix in a new PR?

BBBmau avatar Aug 01 '24 22:08 BBBmau