checkly / checkly-operator

Kubernetes checkly operator
11 stars 3 forks source link

fix(apiChecks): For status code >399 checks should fail #19

Closed akosveres closed 2 years ago

akosveres commented 2 years ago

We're automatically determining if the shouldFail boolean should be true or false based on the expected status code, if it's <400 then it's false, if it's >=400, then it's true.

ianaya89 commented 2 years ago

@akosveres the code looks good to me but I am not sure about this change. Can I ask why you think is this better than rather set the value manually?

akosveres commented 2 years ago

The pattern I've seen is that anything expected above 400 is an expected failure for us. Do you have any example when it is not?

ianaya89 commented 2 years ago

No, I guess that part is correct but I am not entirely getting when this shouldFail function is executed (just trying to understand the flow) and if the DX will be correct. Will it be run before the check creation? If so, wouldn't be hard for the user to understand if the check was set up correctly? For instance, if I made a mistake in the URL (or same check configuration) that makes the check fail, the operator will set shouldFail to true and will look like the check is working fine when is not. I could be wrong but looks like a possible introduction of side effects here.

akosveres commented 2 years ago

The way I understood the logic is:

Is the above assumption wrong?

If it isn't, then this is what I based my work on, the important part is the expected status code, if I'm expecting 403, but the host behind the proxy does not exist, it will get a 404, the check will fail and alerts will be sent out. If the authentication does not work, then a 200 might occur, the check will fail and the alert will be sent out.

akosveres commented 2 years ago

To explain the logic a bit more:

The expected status code is inspected, if it's bellow 400, then shouldFail is automatically false, if it is above, it's true. This is done when we construct the checkly check spec before sending it to the checklyhq API. It's the same for create and update. Update is needed as someone can update the expected status code so we need to re-determine if shouldFail should be false or true.

ianaya89 commented 2 years ago

@akosveres, I am still not 100% sure about this, but I think it is good to go. You are the ones using the operator, I believe you have a good use case behind this 👌

akosveres commented 2 years ago

Okay, let's chat more about it and we can always remove this logic in the future.

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 1.2.5 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: