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

Stable sort release values by key #684

Conversation

longquan0104
Copy link
Contributor

Fixing this issue #675
Helm upgrade always happens without any changes
Clone from #676

Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Overall, it looks good to me but left a few comments for clarification.

Comment on lines +961 to +963
// HelmReleaseChanged returns if the HelmRelease has changed compared to the
// provided values.
func HelmReleaseChanged(hr HelmRelease, revision string, releaseRevision int, valuesChecksums ...string) bool {
Copy link
Contributor

@darkowlzz darkowlzz May 9, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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".

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@darkowlzz darkowlzz May 10, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Do not worry about it, I did this for you.

@longquan0104 longquan0104 force-pushed the bugfix/checksum-chart-values-order-on-key-value branch from 9dbc8b1 to 726730d Compare May 11, 2023 03:29
@hiddeco hiddeco changed the title Bugfix/checksum chart values order on key value Stable order values by key May 11, 2023
@hiddeco hiddeco changed the title Stable order values by key Stable sort values by key May 11, 2023
@hiddeco hiddeco added the enhancement New feature or request label May 11, 2023
@hiddeco hiddeco force-pushed the bugfix/checksum-chart-values-order-on-key-value branch from 726730d to f95fa47 Compare May 11, 2023 08:16
@hiddeco hiddeco changed the title Stable sort values by key Stable sort release values by key May 11, 2023
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 <[email protected]>
Signed-off-by: longquan0104 <[email protected]>
@hiddeco hiddeco force-pushed the bugfix/checksum-chart-values-order-on-key-value branch from f95fa47 to 30b131a Compare May 11, 2023 08:17
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!

@hiddeco hiddeco merged commit 3f2a125 into fluxcd:main May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants