I tend to agree with Wei here, if process can fix the issue maybe it shouldn't go into code.
-- Regards, Aritra Basu On Mon, Apr 15, 2024, 9:42 AM Amogh Desai <amoghdesai....@gmail.com> wrote: > 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] > > > > >
