Adds Blinker dependency which is missing after recent changes #15182
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR fixes a problem introduced by apache#14144 This is a very weird and unforeseen issue. The change introduced a new import from flask `before_render_template` and this caused flask to require `blinker` dependency, even if it was not specified before as 'required' by flask. We have not seen it before, because changes to this part of the code do not trigger K8S tests, however subsequent PRs started to fail because the setup.py did not have `blinker` as dependency. However in CI image `blinker` was installed because it is needed by sentry. So the problem was only detectable in the production image. This is an ultimate proof that our test harness is really good in catchig this kind of errors. The root cause for it is described in https://stackoverflow.com/questions/38491075/flask-testing-signals-not-supported-error Flask support for signals is optional and it does not blinker as dependency, but importing some parts of flask triggers the need for signals.
potiuk
requested review from
ashb,
ephraimbuddy,
jhtimmins,
kaxil,
mik-laj and
turbaszek
ephraimbuddy
approved these changes
Apr 3, 2021
potiuk
added a commit
that referenced
this pull request
Apr 5, 2021
This PR fixes a problem introduced by #14144 This is a very weird and unforeseen issue. The change introduced a new import from flask `before_render_template` and this caused flask to require `blinker` dependency, even if it was not specified before as 'required' by flask. We have not seen it before, because changes to this part of the code do not trigger K8S tests, however subsequent PRs started to fail because the setup.py did not have `blinker` as dependency. However in CI image `blinker` was installed because it is needed by sentry. So the problem was only detectable in the production image. This is an ultimate proof that our test harness is really good in catchig this kind of errors. The root cause for it is described in https://stackoverflow.com/questions/38491075/flask-testing-signals-not-supported-error Flask support for signals is optional and it does not blinker as dependency, but importing some parts of flask triggers the need for signals. (cherry picked from commit 437850b)
ashb
pushed a commit
that referenced
this pull request
Apr 15, 2021
This PR fixes a problem introduced by #14144 This is a very weird and unforeseen issue. The change introduced a new import from flask `before_render_template` and this caused flask to require `blinker` dependency, even if it was not specified before as 'required' by flask. We have not seen it before, because changes to this part of the code do not trigger K8S tests, however subsequent PRs started to fail because the setup.py did not have `blinker` as dependency. However in CI image `blinker` was installed because it is needed by sentry. So the problem was only detectable in the production image. This is an ultimate proof that our test harness is really good in catchig this kind of errors. The root cause for it is described in https://stackoverflow.com/questions/38491075/flask-testing-signals-not-supported-error Flask support for signals is optional and it does not blinker as dependency, but importing some parts of flask triggers the need for signals. (cherry picked from commit 437850b)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a problem introduced by #14144
This is a very weird and unforeseen issue. The change introduced a
new import from flask
before_render_templateand this causedflask to require
blinkerdependency, even if it was notspecified before as 'required' by flask. We have not seen it
before, because changes to this part of the code do not trigger
K8S tests, however subsequent PRs started to fail because
the setup.py did not have
blinkeras dependency.However in CI image
blinkerwas installed because it isneeded by sentry. So the problem was only detectable in the
production image.
This is an ultimate proof that our test harness is really good in
catchig this kind of errors.
The root cause for it is described in
https://stackoverflow.com/questions/38491075/flask-testing-signals-not-supported-error
Flask support for signals is optional and it does not blinker as
dependency, but importing some parts of flask triggers the need
for signals.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.