terraform-provider-helm
terraform-provider-helm copied to clipboard
Enable "helm upgrade --install" equivalent behavior
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
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
Bump: could someone re-approve the checks? I've updated the PR to address the failing ones.
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)
Updated, all tests passing locally.
@bd-spl gentle nudge? :)
@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?
@alexsomesan can I prevail on you to take a look here? I think this is ready to go.
@alexsomesan nudge? :)
@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.
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.
Is there any news on this?
@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.
@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.
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 many thanks, and if I can answer any questions please don't hesitate to ask
@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?
@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?
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.)
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?
@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.
@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.
@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.
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:
- perform the install without the upgrade block or
- 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
- installed
bitnami/redis
with no version set, marking the version as the latest (19.6.1
) - explicitly add
version = 19.6.0
, changing the version19.6.1
->19.6.0
- remove
version
to see whether the version is upgrade to19.6.1
This 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
This is also the reason for Deferred Actions / acceptance_tests
workflow not passing on your PR: https://github.com/orgs/community/discussions/44322
@BBBmau oy gevalt. Well, that's definitely a bit out of scope for this PR to fix. :)
@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 done! I'll need you to approve running the workflows.
@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.
@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 @sheneskaI'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?