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 2022/01/07 13:22:55 UTC
[GitHub] [airflow] Bowrna opened a new pull request #20748: refactoring the code -Breeze2 build ci image
Bowrna opened a new pull request #20748:
URL: https://github.com/apache/airflow/pull/20748
refactoring the code - preventing circular import error, moving out common code to different file
related - https://github.com/apache/airflow/issues/19970
This PR contains a fix. Unpacking the dictionary was missed out. Added in this PR.
<!--
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] potiuk commented on pull request #20748: refactoring the code -Breeze2 build ci image
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20748:
URL: https://github.com/apache/airflow/pull/20748#issuecomment-1008283543
It just needs rebase to get rid of the conflict now, after I merged the previous PR @Bowrna
--
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 a change in pull request #20748: refactoring the code -Breeze2 build ci image
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20748:
URL: https://github.com/apache/airflow/pull/20748#discussion_r780284290
##########
File path: dev/breeze/src/airflow_breeze/global_constants.py
##########
@@ -177,3 +181,41 @@
"POSTGRES_VERSION": "--postgres-version",
"MSSQL_VERSION": "--mssql-version",
}
+
Review comment:
Just make sure `utils/__init__.py` is there too :)
--
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 a change in pull request #20748: refactoring the code -Breeze2 build ci image
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20748:
URL: https://github.com/apache/airflow/pull/20748#discussion_r780280882
##########
File path: dev/breeze/src/airflow_breeze/cache.py
##########
@@ -20,10 +20,10 @@
from typing import Any, List, Optional, Tuple
from airflow_breeze import global_constants
-from airflow_breeze.breeze import get_airflow_sources_root
from airflow_breeze.console import console
+from airflow_breeze.global_constants import BUILD_CACHE_DIR
-BUILD_CACHE_DIR = Path(get_airflow_sources_root(), '.build')
+# BUILD_CACHE_DIR = Path(get_airflow_sources_root(), '.build')
Review comment:
Let's just remove it :)
##########
File path: dev/breeze/src/airflow_breeze/global_constants.py
##########
@@ -177,3 +181,41 @@
"POSTGRES_VERSION": "--postgres-version",
"MSSQL_VERSION": "--mssql-version",
}
+
Review comment:
I tihnk we can move those to a separate file. `global_constants` is not a good place.
maybe `path_utils.py` ? or maybe even we should have "utils" module and move it to "utils/path_utils.py" ?
Then possibly current "utils.py" could be moved to "utils/run_utils.py"
##########
File path: dev/breeze/src/airflow_breeze/global_constants.py
##########
@@ -177,3 +181,41 @@
"POSTGRES_VERSION": "--postgres-version",
"MSSQL_VERSION": "--mssql-version",
}
+
Review comment:
Just make sure `utils/__init__.py` is there too :)
--
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 a change in pull request #20748: refactoring the code -Breeze2 build ci image
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20748:
URL: https://github.com/apache/airflow/pull/20748#discussion_r780280882
##########
File path: dev/breeze/src/airflow_breeze/cache.py
##########
@@ -20,10 +20,10 @@
from typing import Any, List, Optional, Tuple
from airflow_breeze import global_constants
-from airflow_breeze.breeze import get_airflow_sources_root
from airflow_breeze.console import console
+from airflow_breeze.global_constants import BUILD_CACHE_DIR
-BUILD_CACHE_DIR = Path(get_airflow_sources_root(), '.build')
+# BUILD_CACHE_DIR = Path(get_airflow_sources_root(), '.build')
Review comment:
Let's just remove it :)
--
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 a change in pull request #20748: refactoring the code -Breeze2 build ci image
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #20748:
URL: https://github.com/apache/airflow/pull/20748#discussion_r780284011
##########
File path: dev/breeze/src/airflow_breeze/global_constants.py
##########
@@ -177,3 +181,41 @@
"POSTGRES_VERSION": "--postgres-version",
"MSSQL_VERSION": "--mssql-version",
}
+
Review comment:
I tihnk we can move those to a separate file. `global_constants` is not a good place.
maybe `path_utils.py` ? or maybe even we should have "utils" module and move it to "utils/path_utils.py" ?
Then possibly current "utils.py" could be moved to "utils/run_utils.py"
--
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 #20748: refactoring the code -Breeze2 build ci image
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20748:
URL: https://github.com/apache/airflow/pull/20748#issuecomment-1009855228
🎉
--
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 #20748: refactoring the code -Breeze2 build ci image
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20748:
URL: https://github.com/apache/airflow/pull/20748#issuecomment-1007433973
some small comments about the target structure (and tests need fixing too).
--
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 merged pull request #20748: refactoring the code -Breeze2 build ci image
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #20748:
URL: https://github.com/apache/airflow/pull/20748
--
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 #20748: refactoring the code -Breeze2 build ci image
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #20748:
URL: https://github.com/apache/airflow/pull/20748#issuecomment-1007433973
some small comments about the target structure (and tests need fixing too).
--
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