fix: avoid invalid replica in workloadRef

[huikang]

• defaulting replica after workload ref is resolved

Signed-off-by: Hui Kang [email protected]

close #1302

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
• The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
• I've signed my commits with DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My builds are green. Try syncing with master if they are not.
• My organization is added to USERS.md.

[codecov]

Codecov Report

Merging #1304 (1c063ea) into master (96ba030) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1304   +/-   ##
=======================================
  Coverage   81.41%   81.41%           
=======================================
  Files         106      106           
  Lines        9702     9702           
=======================================
  Hits         7899     7899           
  Misses       1266     1266           
  Partials      537      537           

Impacted Files	Coverage Δ
rollout/controller.go	78.35% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96ba030...1c063ea. Read the comment docs.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[jessesuen]

@huikang I'm having trouble understanding how moving the defaulting of spec.replicas fixes the problem and why the problem only happens if there is an analysis in the steps. Could you explain how this fixes it?

[huikang]

@huikang I'm having trouble understanding how moving the defaulting of spec.replicas fixes the problem and why the problem only happens if there is an analysis in the steps. Could you explain how this fixes it?

Sure thing. In the original implementation, if the rollout refers to a deployment and does not have replica set, it will call the update method before resolving the template. Since the template is empty, i.e., no container in the struct), update will return error.

argo-rollouts/rollout/controller.go

Lines 367 to 368 in e1840b5

	r.Spec.Replicas = pointer.Int32Ptr(defaults.DefaultReplicas)
	_, err := c.argoprojclientset.ArgoprojV1alpha1().Rollouts(r.Namespace).Update(ctx, r, metav1.UpdateOptions{})

This PR changes the order of update() and resolve() such that update can return successfully with defaulted replica.
Hope it makes sense to you.

[jessesuen]

Since the template is empty, i.e., no container in the struct), update will return error.

This is what was surprising to me. I didn't think the Update() would fail in that manner. In any case I see this fixes the issue. Thanks!