potiuk commented on code in PR #31984:
URL: https://github.com/apache/airflow/pull/31984#discussion_r1233214890
##########
airflow/providers/microsoft/mssql/hooks/mssql.py:
##########
@@ -63,11 +63,10 @@ def connection_extra_lower(self) -> dict:
@property
def sqlalchemy_scheme(self) -> str:
"""Sqlalchemy scheme either from constructor, connection extras or
default."""
- return (
- self._sqlalchemy_scheme
- or self.connection_extra_lower.get("sqlalchemy_scheme")
- or self.DEFAULT_SQLALCHEMY_SCHEME
- )
+ extra_scheme = self.connection_extra_lower.get("sqlalchemy_scheme")
+ if not self._sqlalchemy_scheme and extra_scheme and (":" in
extra_scheme or "/" in extra_scheme):
Review Comment:
Yes that was a little doubt I had myself and it's a very good point you
raised. I deliberately chose to fail it even if it is not used.
I think it's better to hard-fail in case you have "wrong" extra defined -
even if it is not used. The thing is that you **could** have it defined before
with `;` for whatever reason. But now, if you continue having it there, it
makes it useless:
1) if you have it in constructor, it will not be used
2) if you do not have it in constructor, it will raise an exception
So basically, there is no point whatsoever, to keep it in your extra in case
you have it. It will not be used anyway.
Better to fail it straight away if you have such bad extra, so that you can
fix it quickly.
I am a big fan of failing hard rather than raising warnings, in such case.
While it might be seen as "tough", it's super easy to recover from such error
(just remove the bad extra in one connection and you are done. Having just
warning about it will have no effect in many cases, because people will not do
anything about the warnings, and then it might hit you in the worst time
possible, when someone will write a super-important DAG, and put it in
production and it will start to fail. It's better to immediately fail such bad
configuration at the moment of migration to the new provider version. This time
you are prepared to fix things if needed and you chose the time when to do it,
and you have also option to go back to previous provider. Once you already
migrated and some of your users started to use the new provider features
already and then suddenly you find that something does not work when someone
does not add the scheme in their constructor, you do not have the easy "recovery
" by downgrading the provider.
--
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]
