fix: Use actual weight from status field on rollout object #1937

zachaller · 2022-03-28T14:03:23Z

This fixes #1812 by pulling the actual weight from the status field instead
of the SetWeight.

Signed-off-by: zachaller [email protected]

codecov · 2022-03-28T14:14:22Z

Codecov Report

Merging #1937 (c8095b8) into master (55a041a) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1937      +/-   ##
==========================================
+ Coverage   82.40%   82.42%   +0.02%     
==========================================
  Files         119      119              
  Lines       16910    16913       +3     
==========================================
+ Hits        13934    13940       +6     
+ Misses       2285     2282       -3     
  Partials      691      691

Impacted Files	Coverage Δ
pkg/kubectl-argo-rollouts/info/rollout_info.go	`89.63% <100.00%> (+1.74%)`	⬆️

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 55a041a...c8095b8. Read the comment docs.

This fixes argoproj#1812 by pulling the actual weight from the status field instead of the SetWeight. Signed-off-by: zachaller <[email protected]>

harikrongali · 2022-03-30T17:07:20Z

pkg/kubectl-argo-rollouts/info/rollout_info.go

+				if ro.Status.Canary.Weights != nil {
+					roInfo.ActualWeight = fmt.Sprintf("%d", ro.Status.Canary.Weights.Canary.Weight)
+				} else {
+					roInfo.ActualWeight = "n/a"


instead of n/a, can we set to equal to
roInfo.ActualWeight = roInfo.SetWeight

also, can you verify if we can add a test for this use case?

Yea I can set it back to

roInfo.ActualWeight = roInfo.SetWeight

I went back and for on that and n/a the reason I went with n/a was because I thought it was a bit strange from an end users perspective to have a slightly incorrect value and not knowing its incorrect vs 'n/a' however if you feel it should be n/a I will set it to that.

As for testing I did spend a little time looking into it but found that there was zero tests for rollout_info.go and it seemed a bit odd to test it at the api level when it should really be tested at say the UI side. I did not look if there was UI tests framework so I will spend some time looking into seeing if there is UI testing framework in place that I can use.

@harikrongali tests have been added

…ailabe in status Signed-off-by: zachaller <[email protected]>

Signed-off-by: zachaller <[email protected]>

…tInfoWeights Signed-off-by: zachaller <[email protected]>

Signed-off-by: zachaller <[email protected]>

harikrongali · 2022-03-31T17:21:03Z

pkg/kubectl-argo-rollouts/info/rollout_info.go

@@ -65,7 +65,11 @@ func NewRolloutInfo(
 					}
 				}
 			} else {
-				roInfo.ActualWeight = roInfo.SetWeight
+				if ro.Status.Canary.Weights != nil {
+					roInfo.ActualWeight = fmt.Sprintf("%d", ro.Status.Canary.Weights.Canary.Weight)


this looks good..what happens for bluegreen ? is there a different object with weight?

This if statement can not be hit for blue green and blue green dose not have the ability to have a weight set as well. It dose not exist in the status object.

leoluz · 2022-03-31T17:30:47Z

pkg/kubectl-argo-rollouts/info/info_test.go

+	assert.Equal(t, rolloutObjs.Rollouts[4].Status.Canary.Weights.Canary.Weight, int32(actualWeightStringInt32))
+
+	//This test has a no canary weight object in the status field so we fall back to using SetWeight value
+	roInfo = NewRolloutInfo(rolloutObjs.Rollouts[5], rolloutObjs.ReplicaSets, rolloutObjs.Pods, rolloutObjs.Experiments, rolloutObjs.AnalysisRuns, nil)


More as a suggestion but there is a more interesting pattern to use in tests when you want to validate 2 different scenarios like what you are doing. Instead of mixing the 2 scenarios in one single test case we can leverage testing.Run to run nested executions making more evident what exactly your are testing. See this example.

Advantages are:

Easier to understand what the test is doing

Test description in plain english

Better report when test fail

Running tests in parallel

Signed-off-by: zachaller <[email protected]>

pkg/kubectl-argo-rollouts/info/info_test.go

Signed-off-by: zachaller <[email protected]>

sonarcloud · 2022-03-31T20:01:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

…1937) fix: Use actual weight from status field on rollout object (argoproj#1937) Signed-off-by: zachaller <[email protected]>

…1937) fix: Use actual weight from status field on rollout object (argoproj#1937) Signed-off-by: zachaller <[email protected]> Signed-off-by: Ravi Hari <[email protected]>

fix: Use actual weight from status field on rollout object (#1937) Signed-off-by: zachaller <[email protected]>

Rakhmanov · 2022-07-14T20:38:00Z

Why is it better to look at the status?

Wouldn't it be potentially different (delayed) from desired replica count set in the spec? In cases when you come from the surge of traffic it would be higher when desired (in spec) is lower and after quick scaleup is in order. Status would lag behind? In this scenarios Canary might create not excessive or insufficient number of pods?

* fix: Add pagination to FindLoadBalancerByDNSName (argoproj#1971) * fix: this close issue argoproj#1963 by adding pagination to FindLoadBalancerByDNSName This adds pagination to the FindLoadBalancerByDNSName function this should allow argo rollouts to work with any number of loadbalancers. Signed-off-by: zachaller <[email protected]> * fix: missing lb event (argoproj#2021) * fix: turn missing load balancer log into an event Signed-off-by: zachaller <[email protected]> * consistent naming Signed-off-by: zachaller <[email protected]> * fix: Use actual weight from status field on rollout object (argoproj#1937) fix: Use actual weight from status field on rollout object (argoproj#1937) Signed-off-by: zachaller <[email protected]> * fix: build/lint is broken due to dependencies changes (argoproj#1958) Signed-off-by: zachaller <[email protected]> * following github workflow error prompt to fix Signed-off-by: Travis Perdue <[email protected]> * go fmt Signed-off-by: Travis Perdue <[email protected]> * make go-mod-vendor Signed-off-by: Travis Perdue <[email protected]> * fix path Signed-off-by: Travis Perdue <[email protected]> Signed-off-by: zachaller <[email protected]> Signed-off-by: Travis Perdue <[email protected]> Co-authored-by: Zach Aller <[email protected]> Co-authored-by: Travis Perdue <[email protected]>

…1937) fix: Use actual weight from status field on rollout object (argoproj#1937) Signed-off-by: zachaller <[email protected]> Signed-off-by: Juan Enciso <[email protected]>

fix: Use actual weight from status field on rollout object

39b0fca

This fixes argoproj#1812 by pulling the actual weight from the status field instead of the SetWeight. Signed-off-by: zachaller <[email protected]>

zachaller force-pushed the fix-1812 branch from c300187 to 39b0fca Compare March 28, 2022 14:37

harikrongali reviewed Mar 30, 2022

View reviewed changes

zachaller added 2 commits March 30, 2022 21:52

Per feedback setting to actual weight to roInfo.SetWeight when not av…

94ccef6

…ailabe in status Signed-off-by: zachaller <[email protected]>

fix: add test cases

9c655e8

Signed-off-by: zachaller <[email protected]>

zachaller marked this pull request as draft March 31, 2022 14:21

fix: move to own test

37a33fa

Signed-off-by: zachaller <[email protected]>

zachaller force-pushed the fix-1812 branch from c1da3ca to 37a33fa Compare March 31, 2022 14:47

zachaller added 4 commits March 31, 2022 10:11

fix: have unique names

d6df60a

Signed-off-by: zachaller <[email protected]>

fix: correct all names in resources

16e59f5

Signed-off-by: zachaller <[email protected]>

fix: cleanup copy duplicate test from when I created TestCanaryRollou…

daac8a0

…tInfoWeights Signed-off-by: zachaller <[email protected]>

fix: add comment and cleanup code

72b65f5

Signed-off-by: zachaller <[email protected]>

zachaller marked this pull request as ready for review March 31, 2022 16:20

harikrongali reviewed Mar 31, 2022

View reviewed changes

leoluz reviewed Mar 31, 2022

View reviewed changes

fix: nest testing to provide more detail information around tests

93c20eb

Signed-off-by: zachaller <[email protected]>

leoluz reviewed Mar 31, 2022

View reviewed changes

pkg/kubectl-argo-rollouts/info/info_test.go Outdated Show resolved Hide resolved

fix: spelling

c8095b8

Signed-off-by: zachaller <[email protected]>

zachaller force-pushed the fix-1812 branch from dd0fb32 to c8095b8 Compare March 31, 2022 20:00

harikrongali approved these changes Apr 1, 2022

View reviewed changes

leoluz approved these changes Apr 1, 2022

View reviewed changes

alexmt approved these changes Apr 1, 2022

View reviewed changes

alexmt merged commit 40c22b3 into argoproj:master Apr 1, 2022

zachaller deleted the fix-1812 branch April 1, 2022 17:44

jessesuen pushed a commit that referenced this pull request May 13, 2022

fix: Use actual weight from status field on rollout object (#1937)

9e55f81

fix: Use actual weight from status field on rollout object (#1937) Signed-off-by: zachaller <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Use actual weight from status field on rollout object #1937

fix: Use actual weight from status field on rollout object #1937

zachaller commented Mar 28, 2022

codecov bot commented Mar 28, 2022 •

edited

Loading

harikrongali Mar 30, 2022

harikrongali Mar 30, 2022

zachaller Mar 31, 2022

zachaller Mar 31, 2022

harikrongali Mar 31, 2022

zachaller Mar 31, 2022

leoluz Mar 31, 2022 •

edited

Loading

sonarcloud bot commented Mar 31, 2022

Rakhmanov commented Jul 14, 2022

fix: Use actual weight from status field on rollout object #1937

fix: Use actual weight from status field on rollout object #1937

Conversation

zachaller commented Mar 28, 2022

codecov bot commented Mar 28, 2022 • edited Loading

Codecov Report

harikrongali Mar 30, 2022

Choose a reason for hiding this comment

harikrongali Mar 30, 2022

Choose a reason for hiding this comment

zachaller Mar 31, 2022

Choose a reason for hiding this comment

zachaller Mar 31, 2022

Choose a reason for hiding this comment

harikrongali Mar 31, 2022

Choose a reason for hiding this comment

zachaller Mar 31, 2022

Choose a reason for hiding this comment

leoluz Mar 31, 2022 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Mar 31, 2022

Rakhmanov commented Jul 14, 2022

codecov bot commented Mar 28, 2022 •

edited

Loading

leoluz Mar 31, 2022 •

edited

Loading