-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: Improve checkResourceStatus readability #21260
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Boxuan Tang <[email protected]>
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you fix the typo in the PR title?
Signed-off-by: Boxuan Tang <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #21260 +/- ##
==========================================
+ Coverage 55.18% 55.21% +0.03%
==========================================
Files 337 337
Lines 57058 57049 -9
==========================================
+ Hits 31487 31502 +15
+ Misses 22875 22852 -23
+ Partials 2696 2695 -1 ☔ View full report in Codecov by Sentry. |
healthCheckPassed = healthCheckPassed || healthStatus == string(health.HealthStatusDegraded) | ||
} | ||
} else { | ||
healthCheckPassed = true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be more Go-style to init the variable as true and avoid else block this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for improving this code! LGTM with addressing Andrii's comment.
Signed-off-by: Boxuan Tang <[email protected]>
@@ -2456,28 +2456,21 @@ func checkResourceStatus(watch watchOpts, healthStatus string, syncStatus string | |||
if watch.delete { | |||
return false | |||
} | |||
|
|||
healthBeingChecked := watch.suspended || watch.health || watch.degraded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to init the variable and can just put a condition inline. But it's a small thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I thought it would be more readable that way as it is more obvious that the code is determining if one of the health conditions is being checked. Also happy to inline it if others feel differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk, this way works too.
The previous
checkResourceStatus
function was not very readable as it listed each combination of watch options for health, and this PR aims to simplify and improve the readability of the heath checks incheckResourceStatus
function.