-
Notifications
You must be signed in to change notification settings - Fork 371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable "helm upgrade --install" equivalent behavior #1247
Conversation
As I noted over in the #425, I've released a test build of this for people to kick the tires on: |
a8d9938
to
72b261e
Compare
Bump: could someone re-approve the checks? I've updated the PR to address the failing ones. |
This comment was marked as resolved.
This comment was marked as resolved.
Updated, all tests passing locally. |
@bd-spl gentle nudge? :) |
I welcome this change, albeit have no merging powers here. Please try reaching out the owners of this repository. |
@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. |
3c6f6e8
to
599bca0
Compare
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. |
This PR adds a (potentially) idempotent "upgrade mode" to the provider, mimicing 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 tool boolean values: `enable` and `install`. If `enable` 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.
The failure in acc_test (v0.15.5) does not look like it's in any way obviously relevant to this change; can you try re-running that job? |
Yeah often times the tests can fail due to a network issue. We'll want to add in retries to prevent us from needing to rerun the jobs all the time @JaylonmcShan03 |
@n-oden did the same manual test i did before and I see that it uses the latest version rather than the chart version already being used on the chart: when running 32 [DEBUG] [getInstalledReleaseVersion: mau-test-pr] Chart mau-test-pr is installed as release 19.6.1: timestamp=2024-08-09T15:58:32.743-0700
2024-08-09T15:58:33.849-0700 [INFO] provider.terraform-provider-helm_9.9.9_darwin_arm64: 2024/08/09 15:58:33 [DEBUG] [resourceDiff: mau-test-pr] Got chart redis version 19.6.1: timestamp=2024-08-09T15:58:33.849-0700
2024-08-09T15:58:33.850-0700 [INFO] provider.terraform-provider-helm_9.9.9_darwin_arm64: 2024/08/09 15:58:33 [DEBUG] [resourceDiff: mau-test-pr] Release validated: timestamp=2024-08-09T15:58:33.850-0700
2024-08-09T15:58:33.850-0700 [INFO] provider.terraform-provider-helm_9.9.9_darwin_arm64: 2024/08/09 15:58:33 [DEBUG] [resourceDiff: mau-test-pr] setting version to installed version 19.6.1: timestamp=2024-08-09T15:58:33.850-0700 This appears with this tfconfig where tfstate is empty as well as the release being installe with version being
resource "helm_release" "test" {
name = "mau-test-pr"
chart = "bitnami/redis"
# version = "19.4.0"
upgrade_install = true
} from └─(16:00:30)──> helm list ──(Fri,Aug09)─┘
NAME NAMESPACE REVISION UPDATED STATUS CHART APP VERSION
mau-test-pr default 18 2024-08-09 15:58:07.823949 -0700 PDT deployed redis-19.4.0 7.2.5 |
@BBBmau okay that's weird -- I would think that the test case in TestAccResourceRelease_upgrade_with_install_warmstart_no_version would cover this exact scenario: we preinstall test-chart v1.2.3, apply the upgrade_install plan and then verify that v1.2.3 is still installed. I'll try to figure out what's going on, but can you please attach the full debug log of your tf plan/apply sessions as well as the helm cli commands your ran and their output in a gist? |
@BBBmau I don't seem to be able to reproduce this? Take a look at this gist: https://gist.github.com/n-oden/d54d278d3a2ffbc5e2fb81d6f00d3092 The upshot is:
Can you double-check this on your side? |
One thing that's probably worth explicitly noting is that this does create a new revision, which means that if there are deployments or statefulsets in your release, it will create a kubernetes rollout event, with the associated pod restarts. It's the moral equivalent of running:
...which if you do it locally you will observe does create rollout events for both the deploy and sts resources. I'm going to assert strongly that this is expected behavior and I'll add a note about it to the documentation. |
also use the test repo in the tests for upgrade_install scenarios
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When testing on my machine I was unable to get the behavior despite being able to run the tests and getting them to pass. @jrhouston was able to run manual tests on this PR and it all looks to be running as expected.
@n-oden We really appreciate your contribution! Just apply the documentation changes and we'll get this approved and merged 🎉
docs/index.md
Outdated
* `command` - (Required) Command to execute. | ||
* `args` - (Optional) List of arguments to pass when executing the plugin. | ||
* `env` - (Optional) Map of environment variables to set when executing the plugin. | ||
* `api_version` - (Required) API version to use when decoding the ExecCredentials resource, e.g. `client.authentication.k8s.io/v1beta1`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These extra spaces should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! But tfplugindocs generate
really wants them to be there, and fixing this breaks the documentation check build. I'll revert them out manually but someone should probably figure out why tfplugndocs keeps applying them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah for right now we've been ignoring the documentation workflow due to it generating documentation that's just flat out wrong. Thanks for at least attempting to use it though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big contribution that the community has been asking for for quite sometime. You're more than welcome to pick up other issues that catch your eye if you're interested. Thank you again! 🎉
@BBBmau thank you for all of your help along this long and winding road! Now... ETA to the next release of the provider? 🤣 |
@n-oden Thank you for your contribution 😄 https://github.com/hashicorp/terraform-provider-helm/releases/tag/v2.15.0 |
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.goTo wit: an
upgrade
block is added to the resource attributes, consisting of two boolean values:enable
andinstall
.If
enabled
is true, this causes the provider to create a*action.Upgrade
client, and attempts to perform an upgrade on the named chart. Ifinstall
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
Release Note
Release note for CHANGELOG:
References
Community Note