Skip to content
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

Helm Release detect changes to local chart #2568

Merged
merged 9 commits into from
Sep 17, 2023
Merged

Conversation

EronWright
Copy link
Contributor

@EronWright EronWright commented Sep 14, 2023

Proposed changes

This PR fixes a usability problem with the kubernetes:helm.sh/v3:Release resource when using a local chart. With this fix, Pulumi automatically detects changes to the rendered output of the chart and performs a Helm upgrade accordingly. It does this by persisting a SHA256 checksum of the chart output into the Pulumi stack state.

Users may suppress change detection with the ignoreChanges option, i.e. ignoreChanges: ["checksum"].

Upgrade considerations: after upgrading the provider, users should expect to see a diff (checksum property) and a corresponding Helm upgrade. Helm upgrades are generally idempotent.

Related issues (optional)

Closes #2118

@github-actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2023

Codecov Report

Merging #2568 (523e22a) into master (9453af8) will decrease coverage by 2.45%.
Report is 9 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #2568      +/-   ##
==========================================
- Coverage   21.09%   18.64%   -2.45%     
==========================================
  Files          38       47       +9     
  Lines        8069     9432    +1363     
==========================================
+ Hits         1702     1759      +57     
- Misses       6274     7574    +1300     
- Partials       93       99       +6     
Files Changed Coverage Δ
provider/pkg/provider/helm_release.go 4.39% <0.00%> (-0.10%) ⬇️

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@EronWright EronWright requested a review from a team September 14, 2023 23:15
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have 1 nit and 1 suggestion. I don't have a ton of context on the k8s part, but this seems to do what it claims to.

provider/pkg/provider/helm_release.go Show resolved Hide resolved
provider/pkg/provider/helm_release.go Show resolved Hide resolved
@t0yv0
Copy link
Member

t0yv0 commented Sep 15, 2023

LGTM but I've never worked with this repo before :) Deferring to @rquitales

Copy link
Contributor

@rquitales rquitales left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

// apply ignoreChanges
for _, ignore := range req.GetIgnoreChanges() {
if ignore == "checksum" {
news["checksum"] = oldInputs["checksum"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a testcase here for this functionality as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@EronWright EronWright merged commit 57fa7f7 into master Sep 17, 2023
20 checks passed
@EronWright EronWright deleted the eronwright/issue-2118 branch September 17, 2023 00:30
@EronWright
Copy link
Contributor Author

EronWright commented Sep 18, 2023

Note that this fix applies to the Release resource specifically (not Chart).

EronWright added a commit that referenced this pull request Nov 8, 2023
…2653)

<!--Thanks for your contribution. See [CONTRIBUTING](CONTRIBUTING.md)
    for Pulumi's contribution guidelines.

    Help us merge your changes more quickly by adding more details such
    as labels, milestones, and reviewers.-->

### Proposed changes

This PR passes kubeVersion and apiVersions info from the server to Helm
to fix a regression that occurred in
#2568.

### Related issues (optional)

<!--Refer to related PRs or issues: #1234, or 'Fixes #1234' or 'Closes
#1234'.
Or link to full URLs to issues or pull requests in other GitHub
repositories. -->
Fixes: #2631
EronWright added a commit that referenced this pull request Nov 27, 2023
<!--Thanks for your contribution. See [CONTRIBUTING](CONTRIBUTING.md)
    for Pulumi's contribution guidelines.

    Help us merge your changes more quickly by adding more details such
    as labels, milestones, and reviewers.-->

### Proposed changes

<!--Give us a brief description of what you've done and what it solves.
-->
This PR stabilizes Helm upgrades **by abandoning the use of checksums**
(which was added in v4.5.0, see
[PR](#2568)), while
allowing for local chart change detection via the chart's version
number. That is, the user would be expected to touch the chart's version
to compel Pulumi to roll out the change.

Implementation-wise, this is accomplished by resolving the `version`
property during `Check`. The checked input reflects the goal version,
and if the goal has changed, then `Diff` detects this in the natural way
and effects an upgrade. `Check` no longer uses `helm template` to render
the chart, it simply resolves the chart version.

The diff logic is improved here to use a three-way merge, to be able to
detect drift in the actual Helm release.

The refresh logic is improved to not mutate inputs (as was done in the
past to help with drift detection).

### Related issues (optional)

Closes #2649

<!--Refer to related PRs or issues: #1234, or 'Fixes #1234' or 'Closes
#1234'.
Or link to full URLs to issues or pull requests in other GitHub
repositories. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm Release does not pickup changes in locally stored templates
5 participants