Fix: only recompute metadata when the version in the metadata changes #1458
Conversation
helm/resource_release.go
Outdated
| 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") | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for looking @BBBmau.
terraform-provider-helm/helm/resource_release.go
Lines 1198 to 1209 in 723cb76
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
vprefix is added to or removed from the version inChart.ymlwithout changing the SemVer, but that should be rare and might warrant a reapply.
Should we also do this?
There was a problem hiding this comment.
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
|
@chotiwat can you add in a changelog-entry? I'll be reviewing this PR in the coming days. Thanks! |
|
@BBBmau We are affected by this as it's throwing our CD by constantly producing a false positive change list. |
Description
This PR fixes the issue with
metadatagetting recomputed due to inexactversionattribute, as pointed out by @sheneska in #1344 (comment)Acceptance tests
Release Note
Release note for CHANGELOG:
References
#1344
#1150
Community Note