Summary
#315 adds timeout=getattr(credentials, "login_timeout", None) to every get_token() call. The change is functionally a no-op on every call site, for two independent reasons:
login_timeout does not exist as an attribute on FabricCredentials, so getattr(credentials, "login_timeout", None) always returns None.
get_token() in azure-identity does not accept a timeout argument; it silently disappears into **kwargs and never reaches any underlying HTTP call.
Evidence
The #315 diff shows the parameter being threaded through every get_token() invocation, but dbt/adapters/fabric/fabric_credentials.py does not declare a login_timeout field, and the azure-identity SDK's TokenCredential.get_token signature does not accept a timeout keyword.
User impact
- Users who configured
login_timeout in their profile (or who saw the parameter in release notes and expected it to do something) see no behaviour change.
- The parameter is dead code: it touches every auth path but affects none.
Suggested fix
Either:
- Revert the change, since it has no effect.
- Actually implement a login timeout. That requires (a) adding
login_timeout: Optional[float] = None to FabricCredentials, (b) wrapping get_token() in a real timeout — concurrent.futures.ThreadPoolExecutor + Future.result(timeout=...) is one option, or azure.core.pipeline.policies.RetryPolicy for the underlying HTTP layer.
The first option is preferable: the dbt adapter is not the right layer to invent token-acquisition timeouts when Azure SDK callers don't have a documented timeout knob there.
Happy to open a PR alongside if useful.
Summary
#315 adds
timeout=getattr(credentials, "login_timeout", None)to everyget_token()call. The change is functionally a no-op on every call site, for two independent reasons:login_timeoutdoes not exist as an attribute onFabricCredentials, sogetattr(credentials, "login_timeout", None)always returnsNone.get_token()inazure-identitydoes not accept atimeoutargument; it silently disappears into**kwargsand never reaches any underlying HTTP call.Evidence
The #315 diff shows the parameter being threaded through every
get_token()invocation, butdbt/adapters/fabric/fabric_credentials.pydoes not declare alogin_timeoutfield, and theazure-identitySDK'sTokenCredential.get_tokensignature does not accept atimeoutkeyword.User impact
login_timeoutin their profile (or who saw the parameter in release notes and expected it to do something) see no behaviour change.Suggested fix
Either:
login_timeout: Optional[float] = NonetoFabricCredentials, (b) wrappingget_token()in a real timeout —concurrent.futures.ThreadPoolExecutor+Future.result(timeout=...)is one option, orazure.core.pipeline.policies.RetryPolicyfor the underlying HTTP layer.The first option is preferable: the dbt adapter is not the right layer to invent token-acquisition timeouts when Azure SDK callers don't have a documented timeout knob there.
Happy to open a PR alongside if useful.