Add pre-commits preventing accidental API changes in common.sql #27962
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
potiuk
requested review from
ashb,
mik-laj,
jedcunningham and
eladkal
as code owners
potiuk
force-pushed
the
add-pre-commmits-sql-mypy
branch
from
0bbe3f5
to
7ad141d
Compare
|
The comparision and reporting part is unchanged: Example parameter removal:
|
potiuk
force-pushed
the
add-pre-commmits-sql-mypy
branch
2 times, most recently
from
80ae4a9
to
b6e7a33
Compare
vincbeck
approved these changes
Nov 28, 2022
potiuk
force-pushed
the
add-pre-commmits-sql-mypy
branch
2 times, most recently
from
cfcc168
to
3f03d22
Compare
Fixed and also updated the description a little for the future work. |
vincbeck
reviewed
Nov 28, 2022
potiuk
force-pushed
the
add-pre-commmits-sql-mypy
branch
from
3f03d22
to
36fa4f9
Compare
vincbeck
approved these changes
Nov 28, 2022
|
All looks good :) |
|
🙏 |
kazanzhy
reviewed
Nov 30, 2022
uranusjr
reviewed
Dec 1, 2022
potiuk
force-pushed
the
add-pre-commmits-sql-mypy
branch
2 times, most recently
from
4f7fc72
to
8dad054
Compare
potiuk
force-pushed
the
add-pre-commmits-sql-mypy
branch
from
8dad054
to
c8002a4
Compare
|
OK. No |
ashb
approved these changes
Dec 5, 2022
potiuk
force-pushed
the
add-pre-commmits-sql-mypy
branch
4 times, most recently
from
0d31796
to
2da265b
Compare
We had recently some mishaps when it comes to changes to the common.sql provider that accidentally changed API of the common.sql providers and they passed both tests and reviews. Common.sql is pretty special provider because it used by other providers and we have to make sure the API it epxoses is backwards compatible, otherwise upgrading common.sql provider might break those providers. This had already happened: * The apache#26761 refactored common.sql provided and removed some methods that were actually used by Google Provider * This caused a problem described in apache#27838 and forced us ot yank common.sql 1.3.0 and release 1.3.1 with the methods added back in apache#27843 This change introduces tools and process to flag common.sql API changes that are likely backwards compatible. They will require a deliberate action from the contributor who introduces a breaking API to common.sql, it will also help to flag to reviewer when such a change is made. The idea is based on how Android OS handles flagging API changes for their public APIs paired with the tooling that MyPy provides with generating stubs and verifying usage of packages in the stubs.
potiuk
force-pushed
the
add-pre-commmits-sql-mypy
branch
from
2da265b
to
e0b8dcc
Compare
pierrejeambrun
added
the
changelog:skip
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We had recently some mishaps when it comes to changes to the common.sql provider that accidentally changed API of the common.sql providers and they passed both tests and reviews.
Common.sql is pretty special provider because it used by other providers and we have to make sure the API it epxoses is backwards compatible, otherwise upgrading common.sql provider might break those providers.
This had already happened:
The Common sql bugfixes and improvements #26761 refactored common.sql provided and removed some methods that were actually used by Google Provider
This caused a problem described in apache-airflow-providers-common-sql==1.3.0 breaks BigQuery operators #27838 and forced us ot yank common.sql 1.3.0 and release 1.3.1 with the methods added back in Restore removed (but used) methods in common.sql #27843
This change introduces tools and process to flag common.sql API changes that are likely backwards compatible.
They will require a deliberate action from the contributor who introduces a breaking API to common.sql, it will also help to flag to reviewer when such a change is made.
The idea is based on how Android OS handles flagging API changes for their public APIs paired with the tooling that MyPy provides with generating stubs and verifying usage of packages in the stubs.
^ 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.rstor{issue_number}.significant.rst, in newsfragments.