-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Convert hard-coded allowlist error code to be argument of HttpSensor #33717
Convert hard-coded allowlist error code to be argument of HttpSensor #33717
Conversation
Sure. Good idea. But you need fix static checks (I recommend |
@@ -62,6 +64,9 @@ def response_check(response, task_instance): | |||
:param endpoint: The relative part of the full url | |||
:param request_params: The parameters to be added to the GET url | |||
:param headers: The HTTP headers to be added to the GET request | |||
:param response_error_codes_whitelist: An whitelist to return False on poke(), not to raise exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please change variable name from response_error_codes_whitelist
to response_error_codes_allowlist
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks..! I'll be more careful in naming variable..! 🙇
Thank you for guiding me..! I reflected your feedbacks..! |
You need to add allowlist to spelling.txt :) |
I tried to use
HttpSensor
to roll out our Trino cluster(each with single coordinator) groups periodically with Airflow. In my use case, the503
error has to be in the allowlist, same as404 Not Found
.But, I noticed that
HttpSensor
raiseAirflowException
when the target responds with503 Service Temporarily Unavailable
.How about converting hard-coded
404
to list-typed argumentresponse_error_codes_allowlist
?There is already the alternative option with
extra_option={'check_response': False}
, but the all or nothing method doesn't seem very fancy for covering all the use cases.