SameerMesiah97 commented on code in PR #62731:
URL: https://github.com/apache/airflow/pull/62731#discussion_r2902181100
##########
providers/openfaas/src/airflow/providers/openfaas/hooks/openfaas.py:
##########
@@ -106,7 +109,8 @@ def update_function(self, body: dict[str, Any]) -> None:
"""Update OpenFaaS function."""
url = self.get_conn().host + self.UPDATE_FUNCTION
self.log.info("Updating function %s", url)
- response = requests.put(url, body)
+ timeout = int(self.get_conn().extra_dejson.get("timeout", 60))
Review Comment:
Not against adding timeouts here but there are 2 issues with doing it this
way:
1) The timeout setting is being fetched multiple times in the hook when it's
a constant.
2) You are making timeout configurable via the connection extras, which is
not idiomatic for provider hooks. Also, it does not look like you are exposing
the field via `get_ui_field_behavior` either, so it would become a hidden
setting.
I think it would be better to set a top-level variable like this:
`REQUEST_TIMEOUT = 60`
You can put it near `OK_STATUS_CODE`. You can then pass this variable as an
argument for the `timeout` parameter in this hook.
I do not see a need to expose this setting to users at the moment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
