Add Health class to call health check endpoints on current context #2035
Conversation
k8s-ci-robot
added
the
do-not-merge/invalid-commit-message
k8s-ci-robot
added
needs-rebase
k8s-ci-robot
added
cncf-cla: yes
|
Thanks @cjihrig for your review. I've made the changes, and added a unit test on an uncovered case |
src/health.ts
Outdated
| const status = response.status; | ||
| if (status === 200) { | ||
| return true; | ||
| } else if (status === 404) { |
There was a problem hiding this comment.
remove else since there is return in the preceding case.
src/health.ts
Outdated
| return true; | ||
| } | ||
| return this.healthz(opts); | ||
| } else { |
There was a problem hiding this comment.
remove else since all paths above end in return
|
Thanks for the PR. Minor comments, then LGTM (and special thanks for adding tests!) |
src/health.ts
Outdated
| } catch { | ||
| throw new Error('Error occurred in health request'); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
I get an error when I try to build with this parameter:
npm run build
> @kubernetes/[email protected] build
> tsc
src/health.ts:52:65 - error TS2554: Expected 0-1 arguments, but got 2.
52 throw new Error('Error occurred in health request', { cause: err });
~~~~~~~~~~~~~~
Found 1 error in src/health.ts:52
There was a problem hiding this comment.
Changing the tsconfig with the following would fix the problem, but I'm not sure of the potential side-effects:
"compilerOptions": {
"lib": ["es2022"],
There was a problem hiding this comment.
Ah, ok. There must be a TypeScript setting that needs to be tweaked in the project since cause is a newer addition to the spec. I will resolve my comment.
|
I added a commit, to export the Health class |
|
I also made a change to how the abortcontroller signal was managed. Previously, the abort controller passed to fetch was not accessible. I changed to use the one (if any) passed as parameter, so the caller can abort the request |
feloy
force-pushed
the
feat/health-1.X
branch
from
189c7cd to
15d6e52
Compare
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
setting as draft, the time to make some test, to find the best way for the user to handle abort |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
I made one more change to throw the original error when it is an AbortError, so the caller can detect that the call has been aborted It is ready for review again Sorry for all these afterward changes |
|
Looks good to me. I'd like to get an LGTM from @cjihrig also since he reviewed earlier and then we can merge. |
/assign mstruebing |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, cjihrig, feloy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Add an Health class with 2 public methods
livezandreadyzto check the health check endpoints of the current context.If these endpoints are not served (status 404), the deprecated
healthzis checked. If this endpointhealthzis also not served (status 404), the cluster is considered healthy, else its result is used.