potiuk commented on code in PR #24399:
URL: https://github.com/apache/airflow/pull/24399#discussion_r903450723
##########
setup.cfg:
##########
@@ -153,8 +134,7 @@ install_requires =
pluggy>=1.0
psutil>=4.2.0
pygments>=2.0.1
- # python daemon crashes with 'socket operation on non-socket' for python
3.8+ in version < 2.2.4
- # https://pagure.io/python-daemon/issue/34
+ pyjwt>=2.0.0
Review Comment:
I just wanted to be sure that we force PyJWT to 2+ because of the "require":
["exp", "iat", "nbf"],` change. I consider it very dangerous change and I want
to be sure that we are binding to the right version of PyJWT.
I did not realize that in 2.0 they dropped the "require_iat" aapproach
otherwise in #24519 I would have added `PyJWT<2.0.0` because of that. Luckily
that was in unreleased Airflow so had no impact, but it coul have been very
dangerous if somone by accident had wrong PyJWT.
PyJWT now is much more coupled with us - we are using the PYJWT for signing
and verifying log tokens so it makes perfect sense to bind to it explicitly and
state the minimum version.
BTW, I tend to not describe "why' we added lower-bound as a comment. It's
rather uselss in the "future", because once we lower-bounded something it will
never go down (in most circumstances), so I think it's enough to write >N.N.N
and keep the reason as history in Git. But if you thin it's worth, we can add
also comment here why lower-binding (but I'd prefer to keep the comments when
we have upper binding only and remove the comments when we remove it. This is
much claner and it gives clear information to the "future us" when we can
remove the upper-binidng.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
