Conversation
This commit adds readinessProbe to allow kubernetes decide if requests should be routed to this pod or not. There is also separate e2e test added to verify visibility of /readyz (and already exisiting /healthz) endpoint.
| if !found { | ||
| operatorName = "eunomia-operator" | ||
| } | ||
| operatorNamespace, found := os.LookupEnv("OPERATOR_NAMESPACE") |
There was a problem hiding this comment.
I think the namespace should be retrieved using the method ctx.GetNamespace().
Ref: a41add5
There was a problem hiding this comment.
Hmmm ... no. This namespace, err := ctx.GetNamespace() gets temporary namespace prepared for test, but in this test, I'm testing availability of endpoints of operator itself, so I need its namespace, not temporary one.
There was a problem hiding this comment.
Ahhhh yes, misunderstanding on my part. Thanks!
This commit fixes dependency removed by mistake because of local 1.13 go version. It also fixes service connection initialization problem because of some little time required by Service to obtain connection to operator pod.
I moved service creation at the very beginning of test.
Codecov Report
@@ Coverage Diff @@
## master #287 +/- ##
=====================================
Coverage 0% 0%
=====================================
Files 1 1
Lines 52 52
=====================================
Misses 52 52Continue to review full report at Codecov.
|
akavel
left a comment
akavel
left a comment
There was a problem hiding this comment.
Wow, so few comments from me on this one! ;P
| wantBody: "ok", | ||
| }, | ||
| { | ||
| endpoint: "healthz", |
There was a problem hiding this comment.
this seems to be the only field that changes between the test structs; can we remove all the other ones and just inline them in the body of the for _, tt loop?
There was a problem hiding this comment.
I believe it's good to see clearly what test is trying to verify. I removed requestBody and requestMethod but I think I'd keep want... things. If you think that this is important to remove those as well, let me know.
There was a problem hiding this comment.
[optional] I would personally remove as we talked, but I understand what you mean and leave the choice to you.
In this commit I'm: - fixing issue possibly leading to infinite test run - adding comments explaining some not obvious moments - improving error levels
|
@mkyc I had one inline comment/question here. Let me know what you think. Other than that I think we just need @vinny-sabatini and @akavel to review one more time. |
| wantBody: "ok", | ||
| }, | ||
| { | ||
| endpoint: "healthz", |
There was a problem hiding this comment.
[optional] I would personally remove as we talked, but I understand what you mean and leave the choice to you.
In this commit I'm: - polishing comments even more - fixing error messages as suggested - removing recurrent parameters from test
4 participants
Description
This PR adds readinessProbe to allow kubernetes decide if requests should be routed to this pod or not. There is also separate e2e test added to verify visibility of /readyz (and already exisiting /healthz) endpoint.
Fixes #210
Changes
Type of change
Checklist