-
Notifications
You must be signed in to change notification settings - Fork 163
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 bug checksum chart values will not consider about order of key-value #676
Fix bug checksum chart values will not consider about order of key-value #676
Conversation
Signed-off-by: longquan0104 <[email protected]>
eb8f0ce
to
45c9623
Compare
Hello @stefanprodan, |
Hello @hiddeco, |
Makefile
Outdated
cd api; rm -f go.sum; go mod tidy -compat=1.20 | ||
rm -f go.sum; go mod tidy -compat=1.20 | ||
cd api; rm -f go.sum; go mod tidy -compat=1.19 | ||
rm -f go.sum; go mod tidy -compat=1.19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be changed.
internal/util/util.go
Outdated
case string: | ||
return v | ||
default: | ||
return fmt.Sprintf("%v", v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result of this would be that everything becomes a string type. This may have other undesirable effects, e.g. the change of an integer type to a string (or vice-versa) causing an upgrade to not be triggered.
internal/util/util.go
Outdated
// Marshal | ||
s, err = goyaml.Marshal(msValues) | ||
if err != nil { | ||
panic(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Panics based on arbitrary input with little guarantee it may only occur due to programmatic errors (and not based on user-input) make me feel uncomfortable.
fd1e042
to
984f4a9
Compare
Signed-off-by: longquan0104 <[email protected]>
984f4a9
to
bf7a308
Compare
|
||
goyaml "gopkg.in/yaml.v2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for not using v3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the v2 which is being used in "sigs.k8s.io/yaml" and in the v2, they support MapSlice structure which is easy to handle to me.
Have been thinking some more about this, and while I do think that it would theoretically be better to order keys to prevent unnecessary upgrades. The current way this is structured would cause any existing Given this, I think this needs changing in such a way that this doesn't happen. For example, by calculating the checksum without ordering first, and then only calculate the version with ordering if this causes a mismatch (or when an upgrade is triggered, to then take note of the ordered value). |
So you mean that the key still should be ordered but there should not break the running resources and force them to upgrade? |
This would allow us to eventually stop calculating the unordered checksum, but without triggering an upgrade right now (after upgrading to patched controller with this change). |
Signed-off-by: longquan0104 <[email protected]>
I updated the code, please let me know if there are anything need to be done @hiddeco |
You do not seem to have allowed for us maintainers to push changes to your branch, but please see a332735 for further review reference. |
internal/util/util.go
Outdated
func cleanUpInterfaceMap(in map[interface{}]interface{}) map[string]interface{} { | ||
result := make(map[string]interface{}) | ||
for k, v := range in { | ||
result[fmt.Sprintf("%T.%v", k, k)] = cleanUpMapValue(v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we taking note of the type here? It should take the checksum of an ordered map, without making any modifications to the map itself besides ordering.
Signed-off-by: longquan0104 <[email protected]>
Fixing this issue #675
Helm upgrade always happens without any changes