Add checkReplicasChange function to detect changes in replicas field

[Warashi]

What this PR does:

• Add checkReplicasChange function to detect changes in replicas field
  • there is a bug in original checkReplicasChange, so I backported the fix
• Add support for rendering interface and pointer types in RenderPrimit…
  • We need this because reflect.Interface are returned as their kind when the replicas field is absent either of before/after manifests

Why we need it:

We chose a quick sync strategy when the diffs are only replicas.
We must build a summary message with the workload changed, so we must check it.

Which issue(s) this PR fixes:

Part of #4980

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

• How are users affected by this change:
  When users delete or add the spec.replicas field on the Deployment manifest, they can view the plan summary as expected. Without this PR, they will see an invalid value.

• Is this breaking change: No

• How to migrate (if breaking change): N/A

[Warashi]

🤔 oops, we should put this before as "nil"?
I could not determine whether the empty string or "nil" is better.

[Warashi]

@khanhtc1202 @t-kikuc @ffjlabo
WDYT about this?

[ffjlabo]

@Warashi
When does node.TypeX == nil occur?
I think we should clarify the situation to determine it.

[Warashi]

@ffjlabo Sorry for forgetting to write it.
When the replicas field is not found in the old manifests, node.TypeX becomes nil and node.ValueX becomes invalid value.

[ffjlabo]

@Warashi

@ffjlabo Sorry for forgetting to write it.
When the replicas field is not found in the old manifests, node.TypeX becomes nil and node.ValueX becomes invalid value.

Thanks.

When the replicas field is not found in the old manifests,

It seems that the node is the one that has spec.replicas extracting by the logic before.
So we might not need to check it.

	const replicasQuery = `^spec\.replicas$`
	node, err := ns.FindOne(replicasQuery)

I'm sorry if I misunderstood. 🙏

[Warashi]

Hmm, I think one side of diff defines spec.replicas, and the other side does not; this occurs.

[Warashi]

@ffjlabo
Without this check, this test case fails. (this test is pipedv1's, but it's same logic because this comment thread is on the backported line)
https://github.com/pipe-cd/pipecd/pull/5341/files#diff-4c74652886416486eb7b42cc641e5fd09672015dbaeeb19e3dddefaf81e913dbR1351-R1381

--- FAIL: TestCheckReplicasChange (0.00s)
    --- FAIL: TestCheckReplicasChange/replicas_field_added (0.00s)
        /Users/s01062/ghq/github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/deployment/determine_test.go:1394: 
            	Error Trace:	/Users/s01062/ghq/github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/deployment/determine_test.go:1394
            	Error:      	Not equal: 
            	            	expected: ""
            	            	actual  : "<invalid Value>"
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1 +1 @@
            	            	-
            	            	+<invalid Value>
            	Test:       	TestCheckReplicasChange/replicas_field_added
            	Messages:   	before
FAIL
FAIL	github.com/pipe-cd/pipecd/pkg/app/pipedv1/plugin/kubernetes/deployment	0.954s
FAIL

[ffjlabo]

@Warashi

Hmm, I think one side of diff defines spec.replicas, and the other side does not; this occurs.

I got your point. You are right. Sorry, I misunderstood. 🙏
We need it in such a case.

IMO, It might be better to init before as empty first. I felt easy to guess the value.

before = ""
if node.TypeX != nil {
    // found spec.replicas in the old manifests
    before = node.StringX()
}

[Warashi]

I revised the whole, how about this code?

// checkReplicasChange checks if the replicas field is changed.
func checkReplicasChange(ns diff.Nodes) (before, after string, changed bool) {
	const replicasQuery = `^spec\.replicas$`
	node, err := ns.FindOne(replicasQuery)
	if err != nil {
		// no difference between the before and after manifests, or unknown error occurred.
		return "", "", false
	}

	if node.TypeX == nil {
		// The replicas field is not found in the before manifest.
		// There is difference between the before and after manifests, So it means the replicas field is added in the after manifest.
		return "", node.StringY(), true
	}

	if node.TypeY == nil {
		// The replicas field is not found in the after manifest.
		// There is difference between the before and after manifests, So it means the replicas field is removed in the after manifest.
		return node.StringX(), "", true
	}

	return node.StringX(), node.StringY(), true
}

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 25.34%. Comparing base (00463a6) to head (26616b6).

Files with missing lines	Patch %	Lines
pkg/app/piped/planner/kubernetes/kubernetes.go	27.27%	6 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5341      +/-   ##
==========================================
+ Coverage   25.31%   25.34%   +0.03%     
==========================================
  Files         444      444              
  Lines       47641    47671      +30     
==========================================
+ Hits        12059    12081      +22     
- Misses      34640    34646       +6     
- Partials      942      944       +2     

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