Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add connexion<3.0 upper bound #35218

Merged
merged 3 commits into from
Oct 28, 2023
Merged

Add connexion<3.0 upper bound #35218

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4cc01f4
Add connexion<3.0 upper bound
Kache Oct 27, 2023
1ec459e
Update setup.cfg
potiuk Oct 28, 2023
ddb7267
Update setup.cfg
potiuk Oct 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,13 @@ install_requires =
# Update CustomTTYColoredFormatter to remove
colorlog>=4.0.2, <5.0
configupdater>=3.1.1
connexion[flask]>=2.10.0
# `airflow/www/extensions/init_views` imports `connexion.decorators.validation.RequestBodyValidator`
# connexion v3 has refactored the entire module to middleware, see: /spec-first/connexion/issues/1525
# Specifically, RequestBodyValidator was removed in: /spec-first/connexion/pull/1595
# The usage was added in #30596, seemingly only to override and improve the default error message.
# Either revert that change or find another way, preferably without using connexion internals.
potiuk marked this conversation as resolved.
Show resolved Hide resolved
# This limit can be removed after https://github.com/apache/airflow/issues/35234 is fixed
connexion[flask]>=2.10.0,<3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Usual when we upper bound limit some package we add couple words about what the reason for it, and further steps to remove this limitation.
Could you also add this info?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. We need an explanation for that. Generally you should use constraints where you install airflow from scratch to achieve reproducible builds https://airflow.apache.org/docs/apache-airflow/stable/installation/installing-from-pypi.html - so lack of "upper-binding" is EXPECTED in most dependencies. We need to have a good reason and explanation to upper-bind it.

Copy link
Member

Choose a reason for hiding this comment

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

Or - even better - we need a fix for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'd just thought the git commit message was sufficient. I'll add some source comments

we need a fix for that

Like adding a dynamic branch based on connexion version?

Copy link
Member

Choose a reason for hiding this comment

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

No. Either commit explaining why we need it and what we are waiting for or containing a fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the added comment sufficient? It would be difficult for me to be any more specific, given my limited familiarity with Airflow internals and connexion.

Copy link
Member

Choose a reason for hiding this comment

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

Fix would be better if possible.

cron-descriptor>=1.2.24
croniter>=0.3.17
cryptography>=0.9.3
Expand Down