Skip to content

Conversation

@pankajkoti
Copy link
Member

With PR #34018, the google provider depends on the
common-sql provider changes that are getting released
in the 1.7.2 version of the common-sql provider. Hence,
bump the minimum common-sql provider version to 1.7.2
in the Google provider dependencies.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

With PR apache#34018, the google provider depends on the common-sql
provider changes that are getting released in the 1.7.2 version
of the common-sql provider. Hence, bump the minimum common-sql
provider version to 1.7.2 in the Google provider dependencies.
@boring-cyborg boring-cyborg bot added area:providers provider:google Google (including GCP) related issues labels Sep 10, 2023
@eladkal
Copy link
Contributor

eladkal commented Sep 10, 2023

I may be missing something about our policy but..
Why are we doing this?
Almost every provider release has bug fixes and we don't bump min dependency for the depended provider - We do that only in cases of big impact. There are downsides of bumping min versions.

I'm not saying we shouldn't accept this PR but I'm not sure that a specific bug in (that affect only defer mode!) in a specific operator out of the 50+ operators in Google provider justify bumping min version. The "blast" radius here is very very localized to my taste. Affected users can update providers separately and constraints will use the most recent version anyway.

Why should we limit users who may depend on older versions?
What am I missing here?

@potiuk
Copy link
Member

potiuk commented Sep 10, 2023

TL;DR: I think lack of min-version does not "invalidate" RC1. It might go in the next release. So there is no need to "cancel" google RC1 because of that change. But it is good to have it merged for the next release.

Bumping min version for common-sql is generally "good" . Bumping any "min-version" dependency when we have a good reason for it is generally a GOOD thing. We've been discussing it at several other occasions (azure provider recently) and generally setting "min-version" for a dependncy that we already have higher in our constraints is a good thing - especially if it does not have a potential of conflict. Comon-sql is 1.y.z and we should keep it like that forever, and that means it's backwards-compatible. This means that anyone should be able to safely update to latest 1.* version and there is no potential for conflict.

Good example of such potential conflict - setting pydantic min_version to >= 2.0.0 is a bad idea now as some of our dependencies have not yet upgraded to Pydantic 2. Also Pydantic is the "core" dependency, and we could be far more relaxed in case of provider dependencies. When we set min-version in provider, user can always stick to previous provider version. But this is NOT the case with common.sql - we CAN (and should in this case) bump the min version because potential of conflict is 0. But we do not have to invalidate current release because of that.

@pankajkoti
Copy link
Member Author

Thank you to whoever re-triggered the failed CI jobs :) 🙏🏽

@eladkal
Copy link
Contributor

eladkal commented Sep 10, 2023

I am not convinced yet.
We should increase min versions only when absolutely necessary and we can demonstrate clear value by doing so. The azure package had a clear value - introducing of new functionality and unblocking users who want to use it. In this case we have nothing to say about why we are setting higher lower limit.

You mentioned the conflict part - but that is not the only thing. Providers may be updated seperatly from core. If user will bump google provider you are now also force him to update common.sql provider. That requires more checks, more testing, more time and what if this version of sql provider contains a bug? It happens... Even if we say that everything should be the same, some users take the caution way and sometimes its just the procedure. We should recognize this. I myself been there. I prefer to let users decide when they want to update each provider and not force them into doing something they did not intend unless there is a very good reason.

That said I am -0 here so feel free to merge this one. Mainly because it's common.sql
I wrote this comment just to clarify that what we are going to do here should not be used as the "go to" fix for other providers.

@hussein-awala
Copy link
Member

To be honest, I hesitated before suggesting this for the same reasons, but my assumption was that increasing min_version to a higher minor version (within the same major version) should be safe.

But if you think that's not the case, we can refactor the GCP operator bug fix and introduce a new fix that doesn't need to update the common SQL code. (it's possible with some code duplication, even we can keep the method in common sql, and use it when min_version is > 1.7.2).

@potiuk
Copy link
Member

potiuk commented Sep 10, 2023

You mentioned the conflict part - but that is not the only thing. Providers may be updated seperatly from core. If user will bump google provider you are now also force him to update common.sql provider. That requires more checks, more testing, more time and what if this version of sql provider contains a bug? It happens... Even if we say that everything should be the same, some users take the caution way and sometimes its just the procedure. We should recognize this. I myself been there. I prefer to let users decide when they want to update each provider and not force them into doing something they did not intend unless there is a very good reason.

I disagree. Most users don't care and don't know about transitional dependencies. Common.sql is not useful on it's own. If there are no breaking changes, it does not matter for them. We make the decisions about which transitional dependencies are needed for the "useful" ones. They (at most) care about direct dependendencies ("google" in this case) and they have no idea about the others.

In a way it's contradicting what with "constraints" - with constraints we decide for the users not only what direct dependencies they have but also all 700 transitive ones. With those coonstraints we say the users "you install them and you are good".

What you are really saying is " let's not limit the dependent dependency because it might mean some unexpected errors will happen - AGAINST maintainer intentions". But what "min-version" of common-sql is saying is "we KNOW that if you use older version of common-sql - this feature will not work and we prevent the user from making the mistake". So what we are trading here is "maybe there is some future errorr" vs. "we prevent the user from a known error". I prefer to take the risk - especially that the user can always downgrade google provider if that "uknown" error occurs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants