Skip to content

Conversation

@Dev-iL
Copy link
Collaborator

@Dev-iL Dev-iL commented Jun 25, 2025

closes: #48953, #52663
related: #28723, #50960 (prerequisite that's included in this PR)

  • Add the check_upgrade_sqlalchemy container entrypoint for testing SQLA v2 on CI
  • Fix some existing shellcheck violations reported by shellcheck-py.
  • Robustify multiple unit tests and amend some business logic.

^ 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 airflow-core/newsfragments.

Copilot AI review requested due to automatic review settings June 25, 2025 09:29
@boring-cyborg boring-cyborg bot added area:dev-tools backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch labels Jun 25, 2025

This comment was marked as outdated.

@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch from 96d2e8d to 482fff4 Compare June 25, 2025 09:51
@Dev-iL Dev-iL requested a review from Copilot June 25, 2025 09:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new entrypoint function, check_upgrade_sqlalchemy, to handle upgrading SQLAlchemy for testing SQLA v2 while also addressing several shellcheck violations.

  • Adds check_upgrade_sqlalchemy implementations to both scripts/docker/entrypoint_ci.sh and Dockerfile.ci.
  • Updates Breeze parameters, commands, and CI workflow to include an upgrade_sqlalchemy flag.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
scripts/docker/entrypoint_ci.sh Adds check_upgrade_sqlalchemy function with minor quoting adjustments.
scripts/ci/docker-compose/devcontainer.env Introduces UPGRADE_SQLALCHEMY environment variable.
dev/breeze/src/airflow_breeze/params/shell_params.py Adds new boolean parameter upgrade_sqlalchemy.
dev/breeze/src/airflow_breeze/commands/developer_commands.py Adds upgrade_sqlalchemy option to developer commands.
dev/breeze/src/airflow_breeze/commands/common_options.py Adds a new click option for upgrade_sqlalchemy with help text.
Dockerfile.ci Adds check_upgrade_sqlalchemy function similar to the entrypoint script.
.github/workflows/run-unit-tests.yml Adds upgrade-sqlalchemy input to the CI workflow.
Comments suppressed due to low confidence (1)

dev/breeze/src/airflow_breeze/commands/common_options.py:157

  • [nitpick] The help text for the upgrade_sqlalchemy option mentions an 'experimental supported version'; consider revising it to clearly indicate that it upgrades SQLAlchemy to the latest version required for running tests, ensuring consistency with the function output.
option_upgrade_sqlalchemy = click.option(

@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch 4 times, most recently from d3dd43d to d55a749 Compare June 25, 2025 13:09
@Dev-iL Dev-iL requested a review from vincbeck as a code owner June 25, 2025 13:09
@Dev-iL
Copy link
Collaborator Author

Dev-iL commented Jun 25, 2025

The tests are failing as expected on SQLA v2 incompatibilities. Now what? Should I suggest in #28723 that developers rebase to my branch?

@potiuk
Copy link
Member

potiuk commented Jun 25, 2025

Yes. And make it a part of your PR and we can continuously rebase the PRs on top of yours. This is going to be a little involved and require rebasing here and there.

@Dev-iL Dev-iL requested a review from XD-DENG as a code owner June 25, 2025 15:01
@Dev-iL Dev-iL requested a review from ephraimbuddy as a code owner June 25, 2025 15:47
@Dev-iL
Copy link
Collaborator Author

Dev-iL commented Jun 25, 2025

Should we perhaps decide on a single "main" branch (e.g. your one) and target the various SQLA2 PRs into it? Just to save the effort of rebasing across multiple forks...

@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch 2 times, most recently from 7bde1ac to 34e7502 Compare June 26, 2025 15:43
@Dev-iL Dev-iL marked this pull request as draft June 28, 2025 06:37
@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch 5 times, most recently from 5efe8cb to 9cb0ba2 Compare June 29, 2025 13:44
@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch 6 times, most recently from 67c8353 to 657cce1 Compare August 10, 2025 07:31
@Dev-iL Dev-iL marked this pull request as ready for review August 10, 2025 08:45
Dev-iL added a commit to Dev-iL/airflow that referenced this pull request Aug 10, 2025
This is needed for SQLA2 to work with MySQL 8.4 on python 3.13.

Split from apache#52233 for easier review.
potiuk pushed a commit that referenced this pull request Aug 10, 2025
…54316)

This is needed for SQLA2 to work with MySQL 8.4 on python 3.13.

Split from #52233 for easier review.
@eladkal eladkal removed the backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch label Aug 10, 2025
@eladkal eladkal added this to the Airflow 3.1.0 milestone Aug 10, 2025
Dev-iL and others added 6 commits August 10, 2025 16:34
+ Fix some SQLA2 incompatibilities
+ Skip multiple tests that rely on FAB
Might help with the MySQL 8.4 + Py3.13 combination
- Make some tests more specific so as not trip on irrelevant things.
- Add a couple of fixtures to ensure a predictable engine state.
- Replace uses of unittest.mock with the mocker fixture.
- Add tests for `AutocommitEngineForMySQL`.
@Dev-iL Dev-iL force-pushed the Dev-iL/2506/sqla-v2-tests branch from 657cce1 to f44a247 Compare August 10, 2025 13:35
@potiuk
Copy link
Member

potiuk commented Aug 10, 2025

LGTM. The MyPy fix has been fixed separated (leaving some TODOs: in type checking #54321

@potiuk potiuk merged commit 6f22416 into apache:main Aug 10, 2025
179 of 180 checks passed
@Dev-iL Dev-iL deleted the Dev-iL/2506/sqla-v2-tests branch August 10, 2025 18:20
RoyLee1224 pushed a commit to RoyLee1224/airflow that referenced this pull request Aug 15, 2025
…pache#54316)

This is needed for SQLA2 to work with MySQL 8.4 on python 3.13.

Split from apache#52233 for easier review.
RoyLee1224 pushed a commit to RoyLee1224/airflow that referenced this pull request Aug 15, 2025
* CI: Add SQLA2 core & providers workflows

* Remove the upper limit of SQLA

+ Fix some SQLA2 incompatibilities
+ Skip multiple tests that rely on FAB

* Add a 30s start period to all containers to reduce test flakiness

* Bump mysql-connector-python to 9.1.0

Might help with the MySQL 8.4 + Py3.13 combination

* Set MySQL's `isolation_level` to `AUTOCOMMIT` in SQLA2 during DDL CREATE

* Update the `db.py` tests

- Make some tests more specific so as not trip on irrelevant things.
- Add a couple of fixtures to ensure a predictable engine state.
- Replace uses of unittest.mock with the mocker fixture.
- Add tests for `AutocommitEngineForMySQL`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

all versions If set, the CI build will be forced to use all versions of Python/K8S/DBs area:dev-tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add tests for SQLAlchemy 2

5 participants