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

Fix: only recompute metadata when the version in the metadata changes #1458

Merged
merged 7 commits into from
Sep 20, 2024

Conversation

chotiwat
Copy link
Contributor

@chotiwat chotiwat commented Aug 10, 2024

Description

This PR fixes the issue with metadata getting recomputed due to inexact version attribute, as pointed out by @sheneska in #1344 (comment)

Acceptance tests

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

Release Note

Release note for CHANGELOG:

`resource/helm_release`: Fix: only recompute metadata when the version in the metadata changes [GH-1344](https://github.com/hashicorp/terraform-provider-helm/issues/1344)

References

#1344
#1150

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

@chotiwat chotiwat requested a review from a team as a code owner August 10, 2024 02:36
Comment on lines 859 to 869
if d.HasChange("metadata.0.version") {
// only recompute metadata if the version actually changes
// chart versioning is not consistent and some will add
// a `v` prefix to the chart version after installation
old, new := d.GetChange("version")
old, new := d.GetChange("metadata.0.version")
oldVersion := strings.TrimPrefix(old.(string), "v")
newVersion := strings.TrimPrefix(new.(string), "v")
if oldVersion != newVersion && newVersion != "" {
if oldVersion != newVersion {
d.SetNewComputed("metadata")
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might be able to change this to just

		if d.HasChange("metadata.0.version") {
			d.SetNewComputed("metadata")
		}

which still passes all the acceptance tests.

It will probably trigger changes when the v prefix is added to or removed from the version in Chart.yml without changing the SemVer, but that should be rare and might warrant a reapply.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for opening this PR @chotiwat!

Ideally we would want to apply the logic that helm uses when parsing version strings and do this on state.

metadata is reserved for values that are expected to be computed if changes occur. It's a bit odd to check for changes on a metadata field when the field itself is expected to be computed.

We would accept a PR that applies the parsing logic from helm rather than checking for changes on metadata.0.version. Let me know if you have any other questions.

Copy link
Contributor Author

@chotiwat chotiwat Sep 5, 2024

Choose a reason for hiding this comment

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

Thanks for looking @BBBmau.

return d.Set("metadata", []map[string]interface{}{{
"name": r.Name,
"revision": r.Version,
"namespace": r.Namespace,
"chart": r.Chart.Metadata.Name,
"version": r.Chart.Metadata.Version,
"app_version": r.Chart.Metadata.AppVersion,
"first_deployed": r.Info.FirstDeployed.Time.Unix(),
"last_deployed": r.Info.LastDeployed.Time.Unix(),
"notes": r.Info.Notes,
"values": values,
}})

While metadata is marked as computed in the schema, all of its fields except first_deployed and last_deployed (and maybe notes?) are known at plan time. Inside the resourceDiff function, we already have the expected value for metadata.0.version and it would be the same even if it's recomputed, so I don't see a reason why we cannot use it to detect if the version will actually change.

Could you elaborate more on why you find it odd to do so? Without more context, I feel like it's even odder to have to copy the logic from helm into the provider and re-parse the version when we have that same data in metadata.0.version.

Is there a scenario you think my proposed change would cause an issue? If so, perhaps we could write a test to confirm it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah I see your point. We perform the d.Set("metadata") as part of the releaseRead. From there we perform the releaseDiff which would check for the already set metadata. With this in mind, this is actually fine to include. Thanks for pointing this out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might be able to change this to just

		if d.HasChange("metadata.0.version") {
			d.SetNewComputed("metadata")
		}

which still passes all the acceptance tests.

It will probably trigger changes when the v prefix is added to or removed from the version in Chart.yml without changing the SemVer, but that should be rare and might warrant a reapply.

Should we also do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see your point on setting it but let's leave it how it is for now to prevent the mentioned error from happening, the extra logic was set due to versioning issues in the past the involved missing or extra v

@BBBmau
Copy link
Contributor

BBBmau commented Aug 12, 2024

@chotiwat can you add in a changelog-entry? I'll be reviewing this PR in the coming days. Thanks!

@itay-grudev
Copy link

@BBBmau We are affected by this as it's throwing our CD by constantly producing a false positive change list.

helm/resource_release.go Outdated Show resolved Hide resolved
helm/resource_release.go Outdated Show resolved Hide resolved
Copy link
Contributor

@BBBmau BBBmau left a comment

Choose a reason for hiding this comment

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

This all looks good now, thanks for the contribution as well as your patience. It's appreciated and the discussion was great!

@BBBmau BBBmau merged commit 62d34d2 into hashicorp:main Sep 20, 2024
19 checks passed
@chotiwat chotiwat deleted the fix-metadata-version-diff branch September 20, 2024 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants