Fixing Errors (Update Connexion Library to version 3) #37555

Satoshi-Sh · 2024-02-20T00:33:55Z

Description

This PR was created based on #36052

Todo

Fix the issue that auth_mgr.get_api_endpoints(connexion_app) returns None instead of a blueprint object
Find a way to replace Legacy environ_overrides method from old version of Connexion to fix the problem in tests.
Fix unit tests that use app

^ 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.

vincbeck · 2024-02-20T15:18:41Z

airflow/cli/commands/internal_api_command.py


        init_jinja_globals(flask_app)
        init_xframe_protection(flask_app)
-    return flask_app
+    return connexion_app


Why not returning the flask_app instead of the connexion_app? That would avoid making many changes around this

Thanks, got it. here connexion_app.app returns a FlaskApp so we can return it. Can you explain or give some reference on the benefits you anticipate? We do not have the full context of this PR yet. Will help to understand the migration better 💯

The will decrease the number of changes. There are many changes in this PR which replace cached_app(config={"FAB_UPDATE_PERMS": False}).appbuilder by cached_app(config={"FAB_UPDATE_PERMS": False}).app.appbuilder. By doing so, we will no longer need these changes

Thanks! Got it 🚀

vincbeck · 2024-02-20T15:19:18Z

airflow/cli/commands/internal_api_command.py

-            debug=True,  # nosec
-            use_reloader=not app.config["TESTING"],
+            log_level="debug",
+            # reload=not app.app.config["TESTING"],


What's the change here?

vincbeck · 2024-02-20T15:20:20Z

airflow/providers/fab/auth_manager/fab_auth_manager.py

+        swagger_ui_options = SwaggerUIOptions(
+            swagger_ui=conf.getboolean("webserver", "enable_swagger_ui", fallback=True),
+        )


I think this come from another PR? If so please rebase to make the review easier

vincbeck · 2024-02-20T15:21:20Z

airflow/providers/fab/auth_manager/fab_auth_manager.py

+        )
+
+        connexion_app.extensions["csrf"].exempt(api.blueprint)
+        return api.blueprint if api else None


In which case api can be Falsy?

vincbeck · 2024-02-20T15:25:17Z

I just added few comments. The code changes look massive, some changes do not look related to the PR. Could you please rebase your PR against main?

Also, I think some renaming would help reducing the number of code changes, unless you think this is needed

vincbeck · 2024-02-20T15:25:26Z

Thanks for picking up this effort though!

Satoshi-Sh · 2024-02-20T15:34:53Z

Thanks for the feedback, Vincebeck. This was my first time to use rebase. I think I made some mistakes during the process. I will try it with Jarek tomorrow.

Taragolis · 2024-02-20T18:56:06Z

This was my first time to use rebase.

The first time with git rebase it ever painful, after that things become much easier

vincbeck · 2024-02-20T19:02:35Z

This was my first time to use rebase.

The first time with git rebase it ever painful, after that things become much easier

💯

* Refactor DatasetAll and DatasetAny inheritance They are moved from airflow.models.datasets to airflow.datasets since the intention is to use them with Dataset, not DatasetModel. It is more natural for users to import from the latter module instead. A new (abstract) base class is added for the two classes, plus the OG Dataset class, to inherit from. This allows us to replace a few isinstance checks with simple molymorphism and make the logic a bit simpler. --------- Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> Co-authored-by: Wei Lee <weilee.rx@gmail.com>

…pache#37615) We've been checking - for reproducibility - if python version used is Python 3.9 or above, but since we are also rebuilding sdist packages, we need to check if sdist packages can be converted to wheel packages as well so we need to check python version used.

* Enhancing breeze commands with PACKAGE_LIST env variable * removing from add-back-references as it is idempotent * review comments from potiuk * fixing doc building * fixing static checks

…s to stdout (apache#37439)

The error was misleading by saying envvar should be 'true', instead of the expected value.

…37613) When a task is terminated, it get sent a SIGTERM signal, (through standard_task_runner.terminate). This signal is picked up by taskinstance.py, where it injects an AirflowException into the current running task. The exception raised there is now changed into a new AirflowTaskTerminated exception. This new exception inherits BaseException, so that it is not caught and ignored accidentally by code that tries to perform ordinary error handling. Because termination requests should not be ignored. This will improve reliability of cancelling tasks and will make error logs and error categorization more clear about the reason for failure. (The terminate function does eventually send SIGKILL if the SIGTERM is ignored, but waiting for that should not be necessary) See https://docs.python.org/3/library/exceptions.html#exception-hierarchy

)

…pache#37622)

sudiptob2 · 2024-02-22T16:18:59Z

airflow/providers/fab/auth_manager/fab_auth_manager.py

-        ).blueprint
+        )
+
+        connexion_app.extensions["csrf"].exempt(api.blueprint)


'FlaskApp' object has no attribute 'extensions', So I think it is not possible to do it like this.

Signed-off-by: sudipto baral <sudiptobaral.me@gmail.com>

sudiptob2 · 2024-02-22T17:53:56Z

Hi @vincbeck
we rebased the branch with main. Still, the PR is massive because this PR is based on this made by VladaZakharova. So basically, it contains the entire changes required for upgrading to connexion v3

Can you share your opinion on what should be a proper way to work on this so that we can make a contribution and review process easy?

vincbeck · 2024-02-22T18:28:14Z

Hi @vincbeck we rebased the branch with main. Still, the PR is massive because this PR is based on this made by VladaZakharova. So basically, it contains the entire changes required for upgrading to connexion v3

Can you share your opinion on what should be a proper way to work on this so that we can make a contribution and review process easy?

Unfortunately this PR still contain changes unrelated to your changes. If you look at the commits, I can see for example one commit from me that is now merged in main. I'll try to take a look and see if I can clean this PR

vincbeck · 2024-02-22T18:36:51Z

By looking at it, I think the code changes make sense, just the list of commit does not make sense. As an example I created (and closed) this PR which contain all your changes and only the related commit: #37631

vincbeck · 2024-02-22T18:37:07Z

But I guess it is fair to ignore, git should handle it when we merge

vincbeck · 2024-02-22T18:46:10Z

Hi @vincbeck we rebased the branch with main. Still, the PR is massive because this PR is based on this made by VladaZakharova. So basically, it contains the entire changes required for upgrading to connexion v3

Can you share your opinion on what should be a proper way to work on this so that we can make a contribution and review process easy?

I dont think it is possible to split this PR, everything is connected. Though I think we can reduce the number of changes, the first one would be to not change the returned type of cached_app. I left an line comment. The idea basically is to still returning the Flask app and not the connexion app

Signed-off-by: sudipto baral <sudiptobaral.me@gmail.com>

* Show dataset events above task/run details in grid view * Add extra to dataset event tables

…apache#37623) The version in docs has been out-of-sync with the README. This PR fixes it and adds instruction in release process to keep them in sync.

* Prepare docs ad hoc Teradata provider RC2 February 2024 * Update change log

* Add dataset nodes to dag graph * Clean up dataset nodes

* Initial filter datasets graph by dag_id * Fix bug with merging dataset subgraphs * Fix index for dataset group merging * Add tooltip

Fix GCSObjectsWithPrefixExistenceSensor found prefix in fist poke only in async mode of sensor then it does not push matches in xcom. This PR fix it.

…7633) * when deleting the whole .build folder, the www folder could not be created during asset compilation * -source packages were treated as sdist and we attempted to build wheel files with them * when building airflow packages, default settting (for security reasons) is to build everything in docker container, but building wheel for sdist was done outside. With this PR we only use sdist check when we use local hatch build - and sdist wheel check happens in docker container when sdists are built. * tarball was prepared before the pypi packages and it has been deleted by local hatch build

Satoshi-Sh · 2024-02-22T23:54:48Z

This PR had an issue with rebasing. We moved to #37638 instead of this PR.

boring-cyborg bot added area:API Airflow's REST/HTTP API area:CLI area:providers area:webserver Webserver related Issues kind:documentation provider:fab labels Feb 20, 2024

Satoshi-Sh mentioned this pull request Feb 20, 2024

Attempt to update Connexion library to version 3 #36052

Closed

vincbeck reviewed Feb 20, 2024

View reviewed changes

Ulada Zakharava and others added 7 commits February 21, 2024 22:11

Update methods to use Connexion v3, Ginucorn command and encoding

554fe7b

Fix static checks

9a6b00a

Update setup.cfg and encoding

cfee0cf

Update migrations files for

fd00ba9

Update configuration for tests

f48673d

Fix problem with static checks

1be1a27

Followed vincbeck instruction on the issue apache#36052

912e026

sudiptob2 force-pushed the fix/api-endpoints-registration branch from 7f68766 to 912e026 Compare February 22, 2024 03:21

sunank200 and others added 9 commits February 22, 2024 10:41

Enhancing breeze commands with PACKAGE_LIST env variable (apache#37502)

50edc4e

* Enhancing breeze commands with PACKAGE_LIST env variable * removing from add-back-references as it is idempotent * review comments from potiuk * fixing doc building * fixing static checks

Introduce new config variable to control whether DAG processor output…

5e7bf22

…s to stdout (apache#37439)

Clarify skip-message for test requiring database. (apache#37612)

de059c5

The error was misleading by saying envvar should be 'true', instead of the expected value.

Implement batch_is_authorized_* APIs in AWS auth manager (apache#37430

803e3e1

)

chore: Update comments and logging in OpenLineage ExtractorManager (a…

ec7bccc

…pache#37622)

Merge branch 'main' into fix/api-endpoints-registration

c8b3118

sudiptob2 reviewed Feb 22, 2024

View reviewed changes

sudiptob2 added 2 commits February 22, 2024 11:35

fix: add connexion v3 and missing dependencies in pyproject.toml.

8a3a013

Signed-off-by: sudipto baral <sudiptobaral.me@gmail.com>

fix: use appbuilder.app instead of FlaskApp

8fbded2

Signed-off-by: sudipto baral <sudiptobaral.me@gmail.com>

sudiptob2 deleted the fix/api-endpoints-registration branch February 22, 2024 17:02

sudiptob2 restored the fix/api-endpoints-registration branch February 22, 2024 17:09

sudiptob2 deleted the fix/api-endpoints-registration branch February 22, 2024 17:10

sudiptob2 restored the fix/api-endpoints-registration branch February 22, 2024 17:10

sudiptob2 and others added 10 commits February 22, 2024 16:13

feat: return connexion.app to keep the return type as FlaskApp

a5dfee7

Signed-off-by: sudipto baral <sudiptobaral.me@gmail.com>

Show dataset events above task/run details in grid view (apache#37603)

46274e4

* Show dataset events above task/run details in grid view * Add extra to dataset event tables

Update min kubernetes version in docs and update release instructions (…

4a674b6

…apache#37623) The version in docs has been out-of-sync with the README. This PR fixes it and adds instruction in release process to keep them in sync.

Prepare docs ad hoc Teradata provider RC2 February 2024 (apache#37624)

a236484

* Prepare docs ad hoc Teradata provider RC2 February 2024 * Update change log

Add datasets to dag graph (apache#37604)

e11579e

* Add dataset nodes to dag graph * Clean up dataset nodes

Filter datasets graph by dag_id (apache#37464)

58976f8

* Initial filter datasets graph by dag_id * Fix bug with merging dataset subgraphs * Fix index for dataset group merging * Add tooltip

Fix task duration selection (apache#37630)

c3ca633

Fix Async GCSObjectsWithPrefixExistenceSensor xcom push (apache#37634)

6c01338

Fix GCSObjectsWithPrefixExistenceSensor found prefix in fist poke only in async mode of sensor then it does not push matches in xcom. This PR fix it.

Merge branch 'main' into fix/api-endpoints-registration

affb3d8

Satoshi-Sh closed this Feb 23, 2024

Satoshi-Sh deleted the fix/api-endpoints-registration branch February 23, 2024 00:03

sudiptob2 mentioned this pull request Feb 23, 2024

Migrate to connexion v3 #37638

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing Errors (Update Connexion Library to version 3) #37555

Fixing Errors (Update Connexion Library to version 3) #37555

Satoshi-Sh commented Feb 20, 2024 •

edited

Loading

vincbeck Feb 20, 2024

sudiptob2 Feb 22, 2024

vincbeck Feb 22, 2024

sudiptob2 Feb 22, 2024

vincbeck Feb 20, 2024

vincbeck Feb 20, 2024

vincbeck Feb 20, 2024

vincbeck commented Feb 20, 2024

vincbeck commented Feb 20, 2024 •

edited

Loading

Satoshi-Sh commented Feb 20, 2024

Taragolis commented Feb 20, 2024

vincbeck commented Feb 20, 2024

sudiptob2 Feb 22, 2024

sudiptob2 commented Feb 22, 2024

vincbeck commented Feb 22, 2024

vincbeck commented Feb 22, 2024 •

edited

Loading

vincbeck commented Feb 22, 2024

vincbeck commented Feb 22, 2024

Satoshi-Sh commented Feb 22, 2024

Fixing Errors (Update Connexion Library to version 3) #37555

Fixing Errors (Update Connexion Library to version 3) #37555

Conversation

Satoshi-Sh commented Feb 20, 2024 • edited Loading

Description

Todo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincbeck commented Feb 20, 2024

vincbeck commented Feb 20, 2024 • edited Loading

Satoshi-Sh commented Feb 20, 2024

Taragolis commented Feb 20, 2024

vincbeck commented Feb 20, 2024

Choose a reason for hiding this comment

sudiptob2 commented Feb 22, 2024

vincbeck commented Feb 22, 2024

vincbeck commented Feb 22, 2024 • edited Loading

vincbeck commented Feb 22, 2024

vincbeck commented Feb 22, 2024

Satoshi-Sh commented Feb 22, 2024

Satoshi-Sh commented Feb 20, 2024 •

edited

Loading

vincbeck commented Feb 20, 2024 •

edited

Loading

vincbeck commented Feb 22, 2024 •

edited

Loading