use k8s cli-utils health checks for bundledeployment resources #745
Conversation
internal/healthchecks/builtin.go
Outdated
| isAvailable = &condition | ||
| break | ||
| } | ||
| } | ||
| if conditionExists { | ||
| continue | ||
| if isAvailable == nil { | ||
| gvkErrors = appendResourceError(gvkErrors, obj, "Available condition not found") | ||
| } else if isAvailable.Status == apiregistrationv1.ConditionFalse { | ||
| gvkErrors = appendResourceError(gvkErrors, obj, isAvailable.Message) |
There was a problem hiding this comment.
If you want to avoid the lint error, you could have a boolean variable indicating whether you found the condition, basically a replacement for isAvailable, and then do the check for ConditionFalse within the loop (as in the original code)
ankitathomas
force-pushed
the
k8s_bd_healthchecks
branch
from
0a61bbc
to
ee2131a
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #745 +/- ##
==========================================
- Coverage 24.67% 21.14% -3.54%
==========================================
Files 14 14
Lines 1143 1069 -74
==========================================
- Hits 282 226 -56
+ Misses 807 795 -12
+ Partials 54 48 -6
☔ View full report in Codecov by Sentry. |
|
CodeCov dropped... not sure we care? |
ankitathomas
force-pushed
the
k8s_bd_healthchecks
branch
from
ee2131a
to
279bf2e
Compare
IMO not for the amount it dropped |
|
Removed from merge queue as I saw @ankitathomas pushed just now |
| @@ -135,7 +148,7 @@ func TestAreObjectsHealthy(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| expectedErr: false, | |||
| expectedErr: "", | |||
There was a problem hiding this comment.
TLDR: IMO, checking error strings instead of "an error occurred" is going to result in unit tests that are way to brittle when depending on a third-party library.
While I get the desire to use a string here, I think this is going to result in much more brittle test cases. There was a thread at some point in the #olm-dev channel about this and while I don't have a dog in the fight of "we should vs we shouldn't check error strings in tests", I do think this falls into territory where just checking if an error occurred is more appropriate specifically because a part of the error message is going to be coming from a function call that we don't control (the third-party library). This means that a simple dependency bump could result in unit test failures if they change the values that are returned by result.Message which could realistically be any string.
ankitathomas
force-pushed
the
k8s_bd_healthchecks
branch
from
279bf2e
to
01941c0
Compare
Signed-off-by: Ankita Thomas <[email protected]>
ankitathomas
force-pushed
the
k8s_bd_healthchecks
branch
from
01941c0
to
10a9d2b
Compare
24a4e98
Use the third party kstatus package from sigs.k8s.io/cli-utils for health checks for bundledeployment resources