I agree with the points made by Andrey here. > End users use amazon provider and google provider, and do not use common.sql and both of them have mandatory common.sql as dependency.
On the contrary, would it be better to remove common.sql as mandatory dependencies for providers that do not use it? That way we would avoid one of the bigger problems which is maintaining the providers that have common.sql as dependency but do not need it as and when we someday deprecate common.sql? Thanks & Regards, Amogh Desai On Sun, Apr 14, 2024 at 2:20 PM Wei Lee <weilee...@gmail.com> wrote: > I feel like this is the responsibility of the provider contributor 🤔 > Maybe we could check whether the PR contains common.sql usage and raise a > warning? > > Best, > Wei > > > On Apr 12, 2024, at 12:31 AM, Andrey Anshin <andrey.ans...@taragol.is> > wrote: > > > > There are some drawbacks I saw here, it would force to upgrade other > > providers to the latest version. > > Some scenarios from the my head: > > > > End users use amazon provider and google provider, and do not use > > common.sql and both of them have mandatory common.sql as dependency. > > if everything works fine there is no problem, but it could be a situation > > that a new release of one of the providers is major or contains bugs and > > there is no possible way to downgrade one of them and keep a new version > of > > another without breaking dependencies. > > > > Other case if we need to exclude common.sql provider from provider > release > > wave than we also need to exclude all providers which depends on > common.sql > > even if it problem not affected directly, e.g. it is not a sql only > > provider, e.g. amazon, google, microsoft.azure > > > > If it is not a big deal I have no objections to adding this because I do > > not have a better solution which could be implemented "Here and Now". > > > > It could be resolved if we might run tests against released versions of > > common.sql, but after some brief investigation to run tests > > against installed versions and not a main version this might require > > changing quite a few things, which might take a time. > > > > > > > > > > On Thu, 11 Apr 2024 at 12:41, Jarek Potiuk <ja...@potiuk.com <mailto: > ja...@potiuk.com>> wrote: > > > >> Hello here, > >> > >> I have a proposal to add a general policy that all our providers > depending > >> on common.sql provider always use >= LATEST_MNOR version of the > provider. > >> > >> For example, following the change here > >> https://github.com/apache/airflow/pull/38707 by David we will update > all > >> sql providers to have: airflow-providers-common-sql >= 1.12. We could of > >> course automate that with pre-commit so that we do not have to remember > >> about it. > >> > >> Why ? > >> > >> Because it's very easy by a provider to accidentally use a new feature > in > >> common.sql without bumping the version. Current situation is like in the > >> image attached (thanks David), but we cannot be certain that the > >> "min-versions" there are "good".. > >> > >> A bit more context: > >> > >> Generally speaking - common.sql **should** be backwards compatible - > like > >> - always. We should not make any changes in it's API (which is for-now > >> captured here: > >> > https://github.com/apache/airflow/blob/main/airflow/providers/common/sql/hooks/sql.pyi > >> ). And from time to time we add new things to common.sql that providers > >> might start using. > >> > >> Example: > >> > >> From the > https://lists.apache.org/thread/lzcpllwcgo7pc8g18l3b905kn8f9k4w8 > >> thread the new "suppress_and_warn" method is going to be added. > >> > >> Currently we have no good mechanism to verify if the min version in > >> provider dependencies is really "good". For example when we add > >> `suppress_and_warn` today and someone starts using `suppress_and_warn` > in > >> the "presto" (for example) provider tomorrow, without bumping > common-sql to > >>> = 1.12 - it will fail with 1.11 or earlier installed. > >> > >> On the other hand - all the tests we run in `main` use the "latest" > >> version of the `common.sql` and all the constraints we produce also use > the > >> latest version released, so we can be rather sure that the new providers > >> actually work well with the latest `common.sql` version. If there will > be a > >> bug about compatibility (happened in the past), then it should be fixed > by > >> fixing it in a new patchlevel of the `common.sql`. > >> > >> So in-general it should be "safe" to add >= LATEST MINOR of common.sql > in > >> all providers. > >> > >> Should we do (and automate) it ? Any other comments / proposals maybe ? > >> > >> Here is the current state of dependencies: > >> > >> > >> [image: image.png] > >
