SameerMesiah97 commented on PR #68549: URL: https://github.com/apache/airflow/pull/68549#issuecomment-4789441463
> Thanks for the PR. Sorry for pushing back, but I'm not sure the extraction earns its keep. > > `_SnowflakeOAuthManager` has a single consumer. `SnowflakeHook` creates it once, it's private, and it isn't injected or swappable, so we pay for the indirection without the usual payoffs (reuse, isolation, substitutability). > > The boundary is also leaky. Every method still takes `conn_config` and indexes into it (`account`, `client_id`, `refresh_token`...), and `validate_grant_type` still runs in `_get_conn_params` then again inside `get_valid_oauth_token`. That duplicate validation is relocated, not fixed. > > It's behavior-preserving, so nothing breaks, but it's mostly lateral churn on recently landed code (#60027), and the tests now reach into `hook._oauth.validate_grant_type`, coupling to internals the PR is trying to hide. > > If there's a concrete second consumer or isolation benefit in mind, I'd be glad to hear it. Otherwise I'd lean toward holding off. After considering your argument, I have to agree now that adding a new internal helper class is difficult to justify considering that it is only used by one other class. The motivation for this PR was to group all Oauth related functionality in one part of the hook to improve maintainability and this naturally led me to consider creating `_SnowflakeOAuthManager`. What are your thoughts on simply grouping all Oauth related methods in one part of the hook? I would not open a PR just for something like that but since this has already been opened, are you fine with me changing this PR to group all the Oauth methods within `SnowflakeHook`? -- 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]
