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

Refactor return value of check_for_error to None for all servers #979

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

aleskxyz
Copy link
Contributor

@aleskxyz aleskxyz commented Oct 30, 2023

check_for_error function returns None but some servers are expecting boolean.
I refactor all servers to check the return value explicitly is not None

Fix return value of check_for_error function to send false instead of None
@aleskxyz aleskxyz changed the title Return value of check_for_error Refactor return value of check_for_error to None for all servers Oct 30, 2023
@HenriWahl
Copy link
Owner

HenriWahl commented Oct 30, 2023

@aleskxyz thanks for the quick look for the cause, but in most cases the condition gets quite an opposite result now:

From

if errors_occured:

to

if errors_occured is not None:

there is an important difference, because in the first case it will only return if True, which extends to False in the second variant. I will have to check the consequences if done globally. Maybe as a fix it will be enough if done in Prometheus and Alertmanager.

@aleskxyz
Copy link
Contributor Author

@aleskxyz thanks for the quick look for the cause, but in most cases the condition gets quite an opposite result now:

From

if errors_occured:

to

if errors_occured is not None:

there is an important difference, because in the first case it will only return if True, which extends to False in the second variant. I will have to check the consequences if done globally. Maybe as a fix it will be enough if done in Prometheus and Alertmanager.

@HenriWahl
I can use the first form but it seems we need to revert this PR also: #967

@HenriWahl
Copy link
Owner

OK, I will look for it tomorrow.

@HenriWahl HenriWahl merged commit a9f40f5 into HenriWahl:master Oct 31, 2023
2 checks passed
@HenriWahl
Copy link
Owner

@aleskxyz I checked it looks OK. Thanks for investigating!

@HenriWahl
Copy link
Owner

HenriWahl commented Oct 31, 2023

It's included now in latest release 3.13-20231031.

@aleskxyz
Copy link
Contributor Author

aleskxyz commented Nov 1, 2023

@HenriWahl
You're welcome!
Maybe you need to check these lines. It seems that they don't store the output in any variable to check it later:

self.check_for_error(jsonraw, error, status_code)

self.check_for_error(jsonraw, error, status_code)

self.check_for_error(jsonraw, error, status_code)

self.check_for_error(jsonraw, error, status_code)

self.check_for_error(jsonraw, error, status_code)

self.check_for_error(jsonraw, error, status_code)

self.check_for_error(jsonraw, error, status_code)

@HenriWahl
Copy link
Owner

Well, the whole code needs some refactoring...

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

Successfully merging this pull request may close these issues.

2 participants