pkg/{canary,controller}: remove unused skipLivenessChecks#530
pkg/{canary,controller}: remove unused skipLivenessChecks#530
Conversation
Codecov Report
@@ Coverage Diff @@
## master #530 +/- ##
==========================================
+ Coverage 50.31% 50.50% +0.19%
==========================================
Files 63 63
Lines 4941 4938 -3
==========================================
+ Hits 2486 2494 +8
+ Misses 2044 2034 -10
+ Partials 411 410 -1
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| if cd.Status.Phase == "" || cd.Status.Phase == flaggerv1.CanaryPhaseInitializing { | ||
| if !skipLivenessChecks && !cd.SkipAnalysis() { |
There was a problem hiding this comment.
Removing the SkipAnalysis condition changes the current behaviour. Is this intentional? if so please add a separate commit with it.
There was a problem hiding this comment.
I misunderstood this condition - this is not intentional so I am going to revert this change
There was a problem hiding this comment.
force pushed the reverted version
mathetake
force-pushed
the
remove-skipLivenessChecks-param
branch
from
f67f846 to
65d4b28
Compare
Yes that would be a great improvement to tests readability 👍 |
stefanprodan
left a comment
stefanprodan
left a comment
There was a problem hiding this comment.
LGTM
Thanks @mathetake 🎖
3 participants
I realized that
skipLivenessChecks bool(which equals tojob.SkipTests) has been always set to be false and it's not necessary.So this PR deleted it and fixed tests accordingly
P.S. I think we'd better use gomock of
canary.Controllerandmesh.Routerinterface in order to simplify tests in controller package. Currently they are like small e2e tests and deeply connected with internal implementations of these interfaces, which make it harder for developers who want to contribute to Flagger to understand the internal functionality.