You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/01/17 19:24:54 UTC
[GitHub] [airflow] potiuk opened a new pull request #20910: Switch to non-vendored latest connexion library
potiuk opened a new pull request #20910:
URL: https://github.com/apache/airflow/pull/20910
The `connexion` library has been vendored in because of requests
library that used to have non-optional chardet LGPL dependency,
however requests library had since released a version (which we
helped to provide and convince the requests maintainer to merge)
where chardet is an optional dependency (with mandatory
charset_normalizer). This means that we do not need to vendor-in
connexion any more.
Also connexion after being somewhat abandoned, has been
"revived" and there are active community now that maintains it -
they released several new versions since 2.7.0 we used (2.10.0 is
now the latest version, so we can upgrade to that version instead)
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20910:
URL: https://github.com/apache/airflow/pull/20910#issuecomment-1014923145
Right. Looks like the tests are all green when I replace vendored-in connexion with the latest version from PyPI (the failure was caused by temporary problem with GPG key when building PROD image). Is there anything else (besides running airflow + running some API calls and looking at the Swagger/API docs? Any other tests/expecrations for the connexion/API ?
WDYT @mik-laj @uranusjr @ephraimbuddy ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #20910:
URL: https://github.com/apache/airflow/pull/20910
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #20910:
URL: https://github.com/apache/airflow/pull/20910#issuecomment-1015049048
> Any other tests/expecrations for the connexion/API ?
I think this part of the project has pretty good test coverage and we can rely on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20910:
URL: https://github.com/apache/airflow/pull/20910#discussion_r786253180
##########
File path: setup.cfg
##########
@@ -86,9 +86,8 @@ install_requires =
blinker
cached_property~=1.5;python_version<="3.7"
cattrs~=1.1, !=1.7.*
- # Required by vendored-in connexion
- clickclick>=1.2
Review comment:
@mik-laj @ephraimbuddy - I am not sure if some of those should remain ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #20910:
URL: https://github.com/apache/airflow/pull/20910#discussion_r786406375
##########
File path: setup.cfg
##########
@@ -86,9 +85,8 @@ install_requires =
blinker
cached_property~=1.5;python_version<="3.7"
cattrs~=1.1, !=1.7.*
- # Required by vendored-in connexion
- clickclick>=1.2
colorlog>=4.0.2, <7.0
+ connexion[swagger-ui,flask]>=2.10.0
Review comment:
From I can tell from the source code, the only thing the `flask` extra does is to install Flask, and since Airflow already requires Flask, this extra is essentially no-op. Keeping it is fine IMO since it serves as documentation how we expect to use `connexion`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20910:
URL: https://github.com/apache/airflow/pull/20910#discussion_r786312723
##########
File path: setup.cfg
##########
@@ -86,9 +85,8 @@ install_requires =
blinker
cached_property~=1.5;python_version<="3.7"
cattrs~=1.1, !=1.7.*
- # Required by vendored-in connexion
- clickclick>=1.2
colorlog>=4.0.2, <7.0
+ connexion[swagger-ui,flask]>=2.10.0
Review comment:
Good question - that's why I need some coments from @mik-laj and @ephraimbuddy @uranusjr.
I just added it back from the original connexion vendoring commit: https://github.com/apache/airflow/commit/bb115da0b78036ace5e31e2139c12ca044b3a97d#diff-fa602a8a75dc9dcc92261bac5f533c2a85e34fcceaff63b3a3a81d9acde2fc52L90
My guess it's needed to simply integrate connexion routes with flask.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20910:
URL: https://github.com/apache/airflow/pull/20910#issuecomment-1014930849
BTW the prod image failure is actually due to this one https://github.com/apache/airflow/pull/20889 - once this is merged, prod image should correctly use CI image with "connexion" installed from the PR rather than from "latest" main
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #20910:
URL: https://github.com/apache/airflow/pull/20910#discussion_r786308856
##########
File path: setup.cfg
##########
@@ -86,9 +85,8 @@ install_requires =
blinker
cached_property~=1.5;python_version<="3.7"
cattrs~=1.1, !=1.7.*
- # Required by vendored-in connexion
- clickclick>=1.2
colorlog>=4.0.2, <7.0
+ connexion[swagger-ui,flask]>=2.10.0
Review comment:
What does the flask extra here do?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #20910:
URL: https://github.com/apache/airflow/pull/20910#discussion_r786397877
##########
File path: setup.cfg
##########
@@ -86,9 +85,8 @@ install_requires =
blinker
cached_property~=1.5;python_version<="3.7"
cattrs~=1.1, !=1.7.*
- # Required by vendored-in connexion
- clickclick>=1.2
colorlog>=4.0.2, <7.0
+ connexion[swagger-ui,flask]>=2.10.0
Review comment:
`swagger-ui` is required for docs in WebUI.
https://github.com/apache/airflow/blob/7e29506037fa543f5d9b438320db064cdb820c7b/airflow/www/extensions/init_appbuilder_links.py#L31-L36
`flask` is required, because connexion supports two. framework: `aiohttp` and `flask`, so we need to decide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20910:
URL: https://github.com/apache/airflow/pull/20910#issuecomment-1015390906
Cool. I also tested some basic API calls with the released client, as well as SWAGGER/Redoc and the manual testing also "looks good",
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #20910:
URL: https://github.com/apache/airflow/pull/20910
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #20910:
URL: https://github.com/apache/airflow/pull/20910#issuecomment-1015049354
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #20910: Switch to non-vendored latest connexion library
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #20910:
URL: https://github.com/apache/airflow/pull/20910#discussion_r786401696
##########
File path: setup.cfg
##########
@@ -86,9 +86,8 @@ install_requires =
blinker
cached_property~=1.5;python_version<="3.7"
cattrs~=1.1, !=1.7.*
- # Required by vendored-in connexion
- clickclick>=1.2
Review comment:
We should remove `clickclick` from aiirflow.'s setup.cfg file. Thiis library is only used by connexion CLI. Is not used by Airflow.
https://github.com/zalando/connexion/blob/2066503c5c9f30c2aaf98ad853ff8e16edd64826/connexion/cli.py#L11
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org