potiuk commented on code in PR #44686: URL: https://github.com/apache/airflow/pull/44686#discussion_r1874780209
########## providers/src/airflow/providers/edge/version_compat.py: ########## @@ -0,0 +1,38 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# +# NOTE! THIS FILE IS COPIED MANUALLY IN OTHER PROVIDERS DELIBERATELY TO AVOID ADDING UNNECESSARY +# DEPENDENCIES BETWEEN PROVIDERS. IF YOU WANT TO ADD CONDITIONAL CODE IN YOUR PROVIDER THAT DEPENDS +# ON AIRFLOW VERSION, PLEASE COPY THIS FILE TO THE ROOT PACKAGE OF YOUR PROVIDER AND IMPORT +# THOSE CONSTANTS FROM IT RATHER THAN IMPORTING THEM FROM ANOTHER PROVIDER OR TEST CODE +# + +from __future__ import annotations + +from packaging.version import Version + + +def _get_base_airflow_version() -> Version: + from airflow import __version__ + + return Version(Version(__version__).base_version) Review Comment: Thought about it a bit - and I like the tuple approach more as it does not require top-level imports, which is always a good thing. Also this way I think it would satisfy @dstandish approach where he **only** would like to have `get_airflow_version` (or my equivalent `get_base_airflow_version_tuple` method) that could be easily used without using any predefined constants. Say - you could inline this code then much easier (no additional Version import needed in the provider code): ``` get_airflow_base_version_tuple() > (3, 0, 0) ``` if you do not want to use the constants (though I think constants are a bit better - mostly because it will be easier to remove them as we move the minimum version of airflow up. One benefit of that could be that every time we add new version of Airflow we woud not have to add a new constant (I think @dstandish - this is what you really wanted if I read all your past comment and try to extract the real concerns you had). I propose to merge this one (with the test version_constant separation in #44770 as prerequisite), and maybe as next step if others agree - we could change all provider's code to use such inlined version and change instruction on how we should do it). I am happy to make a draft PR for this approach @dstandish - to see how it would look up. @ashb @gopidesupavan @ashb @jscheffl and others - let me know what you think and whether it is worth to do another follow-up - but let's close this one first to not add confusion. From the very beginning I was not into "let's do it this way" but "let's address the issue we have at hand - for now and the future" - so I am happy to implement it in the way we can all feel comfortable with. I know it took a bit of many people's time, but I think it's important to solve it for the next 8 months to come - while I was making this one pass, I just found - out the 7th (!) way that such check was done inside the openlineage (with even differently named constant used to hold airflow version). -- 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]
