Stable sort release values by key #684
Conversation
| // HelmReleaseChanged returns if the HelmRelease has changed compared to the | ||
| // provided values. | ||
| func HelmReleaseChanged(hr HelmRelease, revision string, releaseRevision int, valuesChecksums ...string) bool { |
There was a problem hiding this comment.
It can be ignored, but would it be helpful for a future reader to provide some context in the function comment about why we allow for multiple valuesChecksums when a HelmRelease can have only one of all the these parameters? We can add some hint about creating room for the possibility of the checksum algorithm change in the future. It's clear now considering the issue we have, but I'm afraid it may be confusing to see multiple values checksums being allowed here. One may be able to check the history and figure it out.
Another way to provide some context here would be to have some tests with multiple values checksums, old and new.
There was a problem hiding this comment.
This code shouldn't continue to exist for too long, as we will be moving away from any of the utility methods in the API package soon anyway. Which is why I simply restructured this to be done with it, while taking into account existing contracts due to SemVer compatibility and "being nice".
internal/util/util.go
Outdated
| // of the order of the keys in the values file. | ||
| func SortMapSlice(ms goyaml.MapSlice) { | ||
| sort.Slice(ms, func(i, j int) bool { | ||
| return fmt.Sprintf("%T.%v", ms[i].Key, ms[i].Key) < fmt.Sprintf("%T.%v", ms[j].Key, ms[j].Key) |
There was a problem hiding this comment.
Please help me understand why the type is being considered here.
In order to understand the reason for the cleanup code below, I went through the rabbit hole of go-yaml/yaml#139 (I think we should link this issue in some comment as a reference to the problem we are trying to address). I also came across different solutions to it like this one from hugo https://github.com/gohugoio/hugo/blob/bcd7ac77043cdf268ef8bf6f2fe33c860c48baaf/parser/metadecoders/decoder.go#L272 .
All these gave me an impression that the key will always be a string. The Key in goyaml MapSlice is an empty interface but when it comes to yamls, I think it'll always be string? Is that incorrect?
There was a problem hiding this comment.
Sorry, that was a mistake from me. I was thinking that the key is an interface{} so when a change in key from one type to another type (e.g., from string to int) should be considered as a change and there should be an upgrade. But after double-checking, when converting to MapSlice the input only takes the Key as a string
There was a problem hiding this comment.
Thanks.
Can you also leave a reference to the go-yaml issue go-yaml/yaml#139 in a comment, maybe around the copyValues() code that iterates on the newValues and cleans it up? Just to have some reference of the issue and upstream discussion around it.
And please avoid pasting the issue link in the commit message to prevent spamming the issue with a referred message, like a lot of others have done 😄 .
Also, I think the changes are ready to be merged. Let's squash the commits, especially the old ones which have irrelevant old changes.
There was a problem hiding this comment.
Sorry for pasting the issue link that makes spam. That shows how amateur I am.
Let's squash the commits Do you mean squash all the commits which include commits of Hideco into one commit or just some nonsense commit of me?
There was a problem hiding this comment.
Do not worry about it, I did this for you.
longquan0104
force-pushed
the
bugfix/checksum-chart-values-order-on-key-value
branch
from
9dbc8b1
to
726730d
Compare
hiddeco
changed the title
hiddeco
force-pushed
the
bugfix/checksum-chart-values-order-on-key-value
branch
from
726730d
to
f95fa47
Compare
This commit changes the way the checksum is calculated for the release values, by stable sorting the keys. By doing this, an upgrade will not be triggered when a key/value pair has just been moved, instead of containing a real change of value. To make it backwards compatible (and without triggering an upgrade due to new ordering), the checksum without ordering is continued to be calculated and compared against until removal in a future controller release. However, only the checksum of the ordered values is taken note of in the Status of the HelmRelease. Co-authored-by: Hidde Beydals <hidde@hhh.computer> Signed-off-by: longquan0104 <longquan0104@gmail.com>
hiddeco
force-pushed
the
bugfix/checksum-chart-values-order-on-key-value
branch
from
f95fa47
to
30b131a
Compare
Fixing this issue #675
Helm upgrade always happens without any changes
Clone from #676