Skip to content
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

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

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

akosveres
Copy link
Collaborator

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.

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.
@akosveres akosveres requested a review from ianaya89 July 11, 2022 11:54
@ianaya89
Copy link

@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
Copy link
Collaborator Author

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
Copy link

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
Copy link
Collaborator Author

The way I understood the logic is:

  • We expect a 403
  • If checkly does the check with no shouldFail set to true, the check fails
  • If we set the check to shouldFail then all is well
  • If the check receives anything other than a 403, even with shouldFail set to true, the check will fail and we can get alerted

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
Copy link
Collaborator Author

akosveres commented Jul 12, 2022

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
Copy link

@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
Copy link
Collaborator Author

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

@akosveres akosveres merged commit d3897e6 into main Jul 14, 2022
@akosveres akosveres deleted the BAU_should_fail branch July 14, 2022 21:06
@github-actions
Copy link

🎉 This PR is included in version 1.2.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants