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 2021/08/23 08:28:38 UTC
[GitHub] [airflow] ashb opened a new pull request #17779: Ensure that airflow modules are not imported too early in conftest
ashb opened a new pull request #17779:
URL: https://github.com/apache/airflow/pull/17779
If someone adds `import airflow` or any submodule in to conftest it will cause airflow.settings and airflow.configuration to be imported and initialized before we set the test env vars -- this leads to _some_ tests failing in bizarre ways.
See https://github.com/apache/airflow/pull/17719#issuecomment-903175600 for one such example
<!--
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] uranusjr commented on pull request #17779: Ensure that airflow modules are not imported too early in conftest
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17779:
URL: https://github.com/apache/airflow/pull/17779#issuecomment-903560897
This seems to break MySQL’s collation settings for some reason
--
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 pull request #17779: Ensure that airflow modules are not imported too early in conftest
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #17779:
URL: https://github.com/apache/airflow/pull/17779#issuecomment-903597055
It _shouldn't_ be able to do that -- all it's doing is checking that something isn't imported too early (which it isn't, otherwise the assert would trigger and the tests would fail early).
But since we don't see the assert I think it has to be something else.
--
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 pull request #17779: Ensure that airflow modules are not imported too early in conftest
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #17779:
URL: https://github.com/apache/airflow/pull/17779#issuecomment-903697418
Yeah, the amount that happens from `import airflow` is a big problem -- both for tests and for "import time"
--
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 #17779: Ensure that airflow modules are not imported too early in conftest
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17779:
URL: https://github.com/apache/airflow/pull/17779#issuecomment-903554985
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] potiuk edited a comment on pull request #17779: Ensure that airflow modules are not imported too early in conftest
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #17779:
URL: https://github.com/apache/airflow/pull/17779#issuecomment-903662619
> This seems to break MySQL’s collation settings for some reason
I thought it could be because it needs to be rebased to latest main. I just merged yesterday a change that removed the need for specifically setting the collation in 5.7, though I still think it **SHOULD** work without rebase, because docker-compose with the parameter + code detecting it in "base.py" were changed together.
BTW. I keep on saying it but, no matter the reason I think this one is only a 'band-aid` - we have much bigger problem with our conf/settings. The way how our conf and settings depend on each other in bizzare and baroque ways continues to amaze me and there is always a bit magic involved regarding the sequence in which it should be imported.
I really think we should finally simplify and use "single responsibility" rule here as now both conf and settings do partially the same and call each-other in cycles. It's really confusing.
It might however break some compatibility.
--
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 merged pull request #17779: Ensure that airflow modules are not imported too early in conftest
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #17779:
URL: https://github.com/apache/airflow/pull/17779
--
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 pull request #17779: Ensure that airflow modules are not imported too early in conftest
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #17779:
URL: https://github.com/apache/airflow/pull/17779#issuecomment-903699807
See https://issues.apache.org/jira/browse/AIRFLOW-1931 :)
--
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 #17779: Ensure that airflow modules are not imported too early in conftest
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17779:
URL: https://github.com/apache/airflow/pull/17779#issuecomment-903662619
> This seems to break MySQL’s collation settings for some reason
I thought it could be because it needs to be rebased to latest main. I just merged yesterday a change that removed the need for specifically setting the collation in 5.7, though I still think it **SHOULD** work without rebase, because docker-compose with the parameter + code detecting it in "base.py" were changed together.
BTW. I keep on saying it but, no matter the reason I think this one is only a 'band-aid` - we have much bigger problem with our conf/settings. The way how our conf and settings depend on each other in bizzare and baroque ways continues to amaze me and there is always a bit magic involved regarding the sequence in which it should be imported.
I really think we should finally simplify and use "single responsibility" rule here as now both conf and settings do partially the same and call each-other. It's really confusing.
It might however break some compatibility.
--
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