Fix the kubernetes manifest diff #5154

Warashi · 2024-08-22T01:32:05Z

What this PR does / why we need it:

Normalize both old and new manifests when calculating diff.

Without this PR, the unexpected difference is calculated when resource/limit numbers have floating-point values such as cpu: "1.5"
The diff signs 1.5 and 1500m are different.
This occurs because the new manifests are normalized, but the old ones are not.

Which issue(s) this PR fixes:

N/A

Does this PR introduce a user-facing change?: Yes

How are users affected by this change:
Users can get the correct plan-preview diffs.
Some deployments triggered by incorrect diffs become not triggered.
Is this breaking change:
How to migrate (if breaking change):

Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>

normalize both of old and new manifests Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>

Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>

codecov · 2024-08-22T01:38:36Z

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 22.82%. Comparing base (c0d5b79) to head (ead1e14).
Report is 116 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/app/piped/platformprovider/kubernetes/diff.go	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5154      +/-   ##
==========================================
+ Coverage   22.79%   22.82%   +0.02%     
==========================================
  Files         412      412              
  Lines       43827    43833       +6     
==========================================
+ Hits         9992    10003      +11     
+ Misses      33047    33040       -7     
- Partials      788      790       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ffjlabo

Thank you! This is the nice improvement!
I left nit comments.

ffjlabo · 2024-08-22T01:49:13Z

pkg/app/piped/platformprovider/kubernetes/diff_test.go

+			for _, change := range result.Changes {
+				t.Log(change.Old.Key, change.New.Key)
+				for _, node := range change.Diff.Nodes() {
+					t.Log(node.PathString)
+					t.Log(node.ValueX)
+					t.Log(node.ValueY)
+					t.Log("---")
+				}
+			}
+			for _, add := range result.Adds {
+				t.Log(add.Key)
+			}
+			for _, delete := range result.Deletes {
+				t.Log(delete.Key)
+			}


[ask] This codes are for checking the diff when result.NoChange() is false?

Yes, and these are logs for debugging, not assertions.

ffjlabo · 2024-08-22T01:52:24Z

pkg/app/piped/platformprovider/kubernetes/diff.go

+
+	normalizedOld, err := remarshal(old.u)
+	if err != nil {
+		logger.Info("compare manifests directly since it was unable to remarshal Kubernetes manifest to normalize special fields", zap.Error(err))


[nit] It would be nice to show old or new when investigating with this log.
It is the same for the remarshal new one.

Suggested change

logger.Info("compare manifests directly since it was unable to remarshal Kubernetes manifest to normalize special fields", zap.Error(err))

logger.Info("compare manifests directly since it was unable to remarshal old Kubernetes manifest to normalize special fields", zap.Error(err))

Fixed by this commit
ead1e14

khanhtc1202

Nice catch, thank you 👍

Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>

ffjlabo

🚀

khanhtc1202

ready to go 🚀

* Add TestLoadAndDiff Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> * Make testcase smaller Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> * Separate test cases Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> * Add test cases Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> * FIx the k8s diff normalize both of old and new manifests Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> * Rename the testcase Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> --------- Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Add note to RELEASE file (#5149) Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Fix the kubernetes manifest diff (#5154) * Add TestLoadAndDiff Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> * Make testcase smaller Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> * Separate test cases Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> * Add test cases Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> * FIx the k8s diff normalize both of old and new manifests Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> * Rename the testcase Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> --------- Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Release v0.48.7 (#5157) Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> --------- Signed-off-by: khanhtc1202 <khanhtc1202@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com> Co-authored-by: Khanh Tran <32532742+khanhtc1202@users.noreply.github.com> Co-authored-by: Shinnosuke Sawada-Dazai <shin@warashi.dev>

Warashi added 5 commits August 22, 2024 10:02

Add TestLoadAndDiff

6cefb64

Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>

Make testcase smaller

595afbb

Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>

Separate test cases

e97698c

Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>

Add test cases

4aa149d

Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>

FIx the k8s diff

eff5db3

normalize both of old and new manifests Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>

Warashi requested review from nghialv, khanhtc1202, kentakozuka, ffjlabo and t-kikuc as code owners August 22, 2024 01:32

github-actions bot added the area/go label Aug 22, 2024

Rename the testcase

282f396

Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>

Warashi enabled auto-merge (squash) August 22, 2024 01:39

ffjlabo reviewed Aug 22, 2024

View reviewed changes

khanhtc1202 previously approved these changes Aug 22, 2024

View reviewed changes

Add words to logs for debugging easily

ead1e14

Signed-off-by: Shinnosuke Sawada <6warashi9@gmail.com>

Warashi dismissed khanhtc1202’s stale review via ead1e14 August 22, 2024 02:00

Warashi requested review from ffjlabo and khanhtc1202 August 22, 2024 02:03

ffjlabo approved these changes Aug 22, 2024

View reviewed changes

khanhtc1202 approved these changes Aug 22, 2024

View reviewed changes

Warashi merged commit c24ce35 into master Aug 22, 2024
18 checks passed

Warashi deleted the kustomize-limits-diff branch August 22, 2024 02:11

github-actions bot mentioned this pull request Aug 26, 2024

Release v0.48.7 #5157

Merged

Warashi added cherry-pick v0.48.7 labels Aug 26, 2024

github-actions bot mentioned this pull request Aug 26, 2024

Cherry-pick #5154 #5159

Closed

github-actions bot mentioned this pull request Aug 26, 2024

Cherry-pick #5149 #5154 #5157 #5160

Merged

This was referenced Sep 3, 2024

Update release to v0.48.8 #5176

Merged

Release v0.48.9 #5205

Merged

github-actions bot mentioned this pull request Sep 24, 2024

Update RELEASE to v0.49.0 #5221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the kubernetes manifest diff #5154

Fix the kubernetes manifest diff #5154

Warashi commented Aug 22, 2024 •

edited

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading

ffjlabo left a comment

ffjlabo Aug 22, 2024

Warashi Aug 22, 2024

ffjlabo Aug 22, 2024

Warashi Aug 22, 2024

khanhtc1202 left a comment

ffjlabo left a comment

khanhtc1202 left a comment

	logger.Info("compare manifests directly since it was unable to remarshal Kubernetes manifest to normalize special fields", zap.Error(err))
	logger.Info("compare manifests directly since it was unable to remarshal old Kubernetes manifest to normalize special fields", zap.Error(err))

Fix the kubernetes manifest diff #5154

Fix the kubernetes manifest diff #5154

Conversation

Warashi commented Aug 22, 2024 • edited Loading

codecov bot commented Aug 22, 2024 • edited Loading

Codecov Report

ffjlabo left a comment

Choose a reason for hiding this comment

ffjlabo Aug 22, 2024

Choose a reason for hiding this comment

Warashi Aug 22, 2024

Choose a reason for hiding this comment

ffjlabo Aug 22, 2024

Choose a reason for hiding this comment

Warashi Aug 22, 2024

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

Warashi commented Aug 22, 2024 •

edited

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading