-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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.
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
Get airflow version from importlib.metadata rather than hard-coding #12786
Conversation
One less thing to change, and one less pre-commit step needed :)
|
||
version = metadata.version('apache-airflow') | ||
|
||
del metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This line isn't strictly needed, it was just so that the only symbol in this module was version
. For no god reason really)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! This is so much better in this direction than previously !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked more closely and we need to change it in few more places. Comment follows.
We should fix it in few other places as well:
|
Cool, I'll update those use cases. |
We could use Pushed a fixup with that -- lets see what happens. |
# Read airflow version from the version.py | ||
AIRFLOW_VERSION=$(grep version "${AIRFLOW_SOURCES}/airflow/version.py" | awk '{print $3}' | sed "s/['+]//g") | ||
# Read airflow version from the setup.py. | ||
AIRFLOW_VERSION=$(python "${AIRFLOW_SOURCES}/setup.py" --version) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to run Python here if possible. While this is a reasonable expectation that Python is installed in the hos, there are still some cases where people will have python2 by default for example and i would rather keep it bash only. There is another case in the future where we will use those scripts in code spaces environment and vs code to run docker-only environment and i would not like to have to depend on Python there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scripts/ci/libraries/_initialization.sh: line 138: python: command not found
Failed anyway :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course that all untill we rewrite breeze in python - which might change the whole approach: #12282
As commented - i'd prefer base it on having POSIX compliant OS simply. |
I'm not sure, but Google tried to use a similar trick and it causes problems. |
That seems specific to using Pyinstaller |
Yeah. I would not worry about it. At worst, it might only cause problems in the dev environment. If we find some, we can always catch any exceptions and make a workaround for dev-only env. |
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 master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
One less thing to change, and one less pre-commit step needed :)
^ 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.