Fixed a bug in #4199 that resulted in wrong Out of Sync. #4248
Conversation
|
@knanao |
| @@ -53,19 +53,19 @@ type DiffListChange struct { | |||
| func Diff(old, new Manifest, logger *zap.Logger, opts ...diff.Option) (*diff.Result, error) { | |||
| if old.Key.IsSecret() && new.Key.IsSecret() { | |||
| var err error | |||
| new.u, err = normalizeNewSecret(old.u, new.u) | |||
| old.u, err = normalizeNewSecret(old.u, new.u) | |||
There was a problem hiding this comment.
sorry but why does normalizeNewSecret return old stub 🤔 This does not make sense to me 👀
There was a problem hiding this comment.
In a nutshell, it is because we swapped the order of headManifest and liveManifest in Diff.
The ignoreAddingMapKeys in diff/diff.go is to ignore LiveManifest and normalizeNewSecret depends on to ignore LiveManifest with ignoreAddingMapKeys.
There was a problem hiding this comment.
Thanks for clarifying, then in that case, why not just mention directly here that it's Live/Head manifest or, even better Base/Compare instead of Old/New one here? 🤔 I'm not talking about the logic but the readability of the code, return value from normalizeNewSecret is set to old one doesn't make sense to me, that's my point 👀
There was a problem hiding this comment.
I see what you mean. I too have a problem with the fact that old/new is not explicitly tied to live/head.
However, since the relationship between old/new and live/head is not only in this section, but is tied across the board, I also think that a separate Issue or PR should be created to rename it.
Or is your point that normalizeNewSecret should be normalizeOldSecret? If so, I will create an improved PR immediately.
There was a problem hiding this comment.
I also think that a separate Issue or PR should be created to rename it.
Yes, It's what I mean too 👍 The logic around this part is tricky, and if the flow & naming are hard to follow, it's hard for us to debug next time problems occur around here.
There was a problem hiding this comment.
Ok, I will create this issue. Thank you for nice comment:smile:
What this PR does / why we need it:
Fixed a bug in #4199 that resulted in wrong Out of Sync.
Also, during this modification, the diff option WithIgnoreAddingMapKeys was made commutative. That is, diff a b = diff b a.
I would like to discuss the validity of this modification.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: