Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
gopidesupavan commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2762240102 LGTM overall, only few nits. Good work :) -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
potiuk merged PR #48099: URL: https://github.com/apache/airflow/pull/48099 -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
jedcunningham commented on code in PR #48099: URL: https://github.com/apache/airflow/pull/48099#discussion_r2019473943 ## airflow-ctl/src/airflow/ctl/__main__.py: ## @@ -23,7 +23,7 @@ import argcomplete -from airflowctl.ctl import cli_parser +from airflow.ctl.cli import cli_parser Review Comment: Just dropping a "request changes" review here so we don't forget to move this back to `airflowctl`. -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
gopidesupavan commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019185630
##
Dockerfile.ci:
##
@@ -589,7 +589,7 @@ function install_airflow() {
# We do not yet use ``uv sync`` because we are not committing and
using uv.lock yet and uv sync
# cannot use constraints - once we switch to uv.lock (with the
workflow that dependabot will update it
# and constraints will be generated from it, we should be able to
simply use ``uv sync`` here
-installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./devel-common
--editable ./airflow-core"
+installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./airflow-ctl
--editable ./devel-common"
Review Comment:
you can update in one of the file, pre-commit will update other file.
--
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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
potiuk commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2765987296 > > Rebased now as there were some ms dependency issues. > > Many thanks, @potiuk for your help in this! Also for accepting to rebase from your PR :) No worries. Already done :) -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2765869792 > Rebased now as there were some ms dependency issues. Many thanks, Jarek! -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
potiuk commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2765573918 I dismissed yours @jedcunningham -> your concern was definiely addressed :) -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
potiuk commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2765572115 @jedcunningham ? -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
potiuk commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2764865862 BTW. There should not be many conflicts with #48223 -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
potiuk commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2764922371 Generally, it looks good. Re-reviewed it, and you can even merge it @bugraoz93 -> once @jedcunningham confirms that he is ok . I will take care about resolving conflicts with #48223 -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
potiuk commented on code in PR #48099: URL: https://github.com/apache/airflow/pull/48099#discussion_r2020329025 ## .github/workflows/airflow-distributions-tests.yml: ## @@ -16,10 +16,14 @@ # under the License. Review Comment: nice colution! -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
potiuk commented on code in PR #48099: URL: https://github.com/apache/airflow/pull/48099#discussion_r2020307555 ## .github/workflows/airflow-distributions-tests.yml: ## @@ -16,10 +16,14 @@ # under the License. Review Comment: > cc @potiuk if you have any suggestion on how best to handle this limitation (or any way to bump that number for us -- if that is possible) Unfottunately this limitation is hard-coded in GitHub -> https://github.com/orgs/community/discussions/32192 -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
potiuk commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2764861052 You will have to rebase some of the changes after https://github.com/apache/airflow/pull/48223 @bugraoz93 -> a numbe of the changes there are somewhat overlapping with this change (mypy changes already include airflow-ctil) - I suggest to merge #48233 , rebase and then I am happy to review/help this one. -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
potiuk commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2764865402 Rebased now as there were some ms dependency issues. -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
potiuk commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2764864993 And it looks cool :) -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on code in PR #48099: URL: https://github.com/apache/airflow/pull/48099#discussion_r2019884059 ## airflow-ctl/src/airflow/ctl/__main__.py: ## @@ -23,7 +23,7 @@ import argcomplete -from airflowctl.ctl import cli_parser +from airflow.ctl.cli import cli_parser Review Comment: Reverted back to `airflow-ctl/src/airflowctl` :) -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on code in PR #48099: URL: https://github.com/apache/airflow/pull/48099#discussion_r2019882986 ## .github/workflows/airflow-distributions-tests.yml: ## @@ -16,10 +16,14 @@ # under the License. Review Comment: Updated with the suggested approach -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019883565
##
Dockerfile.ci:
##
@@ -589,7 +589,7 @@ function install_airflow() {
# We do not yet use ``uv sync`` because we are not committing and
using uv.lock yet and uv sync
# cannot use constraints - once we switch to uv.lock (with the
workflow that dependabot will update it
# and constraints will be generated from it, we should be able to
simply use ``uv sync`` here
-installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./devel-common
--editable ./airflow-core"
+installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./airflow-ctl
--editable ./devel-common"
Review Comment:
Yes, I had hard times once and learned that pre-commit :)
##
dev/breeze/src/airflow_breeze/utils/packages.py:
##
@@ -843,6 +843,9 @@ def regenerate_pyproject_toml(
in_optional_dependencies = False
in_additional_devel_dependency_groups = False
for line in pyproject_toml_content.splitlines():
+print(f"LINE: {line}")
+if "ctl" in line:
Review Comment:
Removed
--
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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019804908
##
dev/breeze/src/airflow_breeze/utils/packages.py:
##
@@ -843,6 +843,9 @@ def regenerate_pyproject_toml(
in_optional_dependencies = False
in_additional_devel_dependency_groups = False
for line in pyproject_toml_content.splitlines():
+print(f"LINE: {line}")
+if "ctl" in line:
Review Comment:
I was testing something and forgot to delete this. Later, I found where the
uv workspaces are generated :) I will remove it with the print above. Great one
--
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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019804908
##
dev/breeze/src/airflow_breeze/utils/packages.py:
##
@@ -843,6 +843,9 @@ def regenerate_pyproject_toml(
in_optional_dependencies = False
in_additional_devel_dependency_groups = False
for line in pyproject_toml_content.splitlines():
+print(f"LINE: {line}")
+if "ctl" in line:
Review Comment:
I was testing something and forgot to delete this. Later, I found where the
uv workspaces are generated :) I will remove it. Great one
--
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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on code in PR #48099: URL: https://github.com/apache/airflow/pull/48099#discussion_r2019803307 ## .github/workflows/airflow-distributions-tests.yml: ## @@ -16,10 +16,14 @@ # under the License. Review Comment: Thanks @gopidesupavan! Great idea. I agree. This will make things more dynamic so that no need to duplicate steps inside the workflow itself. I will include this change with the other path changes. -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
gopidesupavan commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019209957
##
dev/breeze/src/airflow_breeze/utils/packages.py:
##
@@ -843,6 +843,9 @@ def regenerate_pyproject_toml(
in_optional_dependencies = False
in_additional_devel_dependency_groups = False
for line in pyproject_toml_content.splitlines():
+print(f"LINE: {line}")
+if "ctl" in line:
Review Comment:
Not sure this may cause side effect with normal package names?
--
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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
gopidesupavan commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019212973
##
scripts/ci/testing/run_unit_tests.sh:
##
@@ -130,13 +130,24 @@ function task_sdk_tests() {
}
+function airflow_ctl_tests() {
+echo "${COLOR_BLUE}Running Task SDK tests${COLOR_RESET}"
+set -x
+breeze testing airflow-ctl-tests
+set +x
+echo "${COLOR_BLUE}Task SDK tests completed${COLOR_RESET}"
Review Comment:
```suggestion
echo "${COLOR_BLUE}Airflow CTL tests completed${COLOR_RESET}"
```
--
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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
gopidesupavan commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019213659
##
scripts/docker/install_airflow.sh:
##
@@ -51,7 +51,7 @@ function install_airflow() {
# We do not yet use ``uv sync`` because we are not committing and
using uv.lock yet and uv sync
# cannot use constraints - once we switch to uv.lock (with the
workflow that dependabot will update it
# and constraints will be generated from it, we should be able to
simply use ``uv sync`` here
-installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./devel-common
--editable ./airflow-core"
+installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./airflow-ctl
--editable ./devel-common"
Review Comment:
```suggestion
installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./airflow-ctl
--editable ./devel-common"
```
--
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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
gopidesupavan commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019212471
##
scripts/ci/testing/run_unit_tests.sh:
##
@@ -130,13 +130,24 @@ function task_sdk_tests() {
}
+function airflow_ctl_tests() {
+echo "${COLOR_BLUE}Running Task SDK tests${COLOR_RESET}"
Review Comment:
```suggestion
echo "${COLOR_BLUE}Running Airflow CTL tests${COLOR_RESET}"
```
--
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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
gopidesupavan commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019204888
##
dev/breeze/src/airflow_breeze/utils/packages.py:
##
@@ -843,6 +843,9 @@ def regenerate_pyproject_toml(
in_optional_dependencies = False
in_additional_devel_dependency_groups = False
for line in pyproject_toml_content.splitlines():
+print(f"LINE: {line}")
Review Comment:
does this print required? if yes you can use `get_console().print()`
--
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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
gopidesupavan commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019184804
##
Dockerfile.ci:
##
@@ -589,7 +589,7 @@ function install_airflow() {
# We do not yet use ``uv sync`` because we are not committing and
using uv.lock yet and uv sync
# cannot use constraints - once we switch to uv.lock (with the
workflow that dependabot will update it
# and constraints will be generated from it, we should be able to
simply use ``uv sync`` here
-installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./devel-common
--editable ./airflow-core"
+installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./airflow-ctl
--editable ./devel-common"
Review Comment:
```suggestion
installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./airflow-ctl
--editable ./devel-common"
```
--
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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
gopidesupavan commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019184022
##
Dockerfile:
##
@@ -834,7 +834,7 @@ function install_airflow() {
# We do not yet use ``uv sync`` because we are not committing and
using uv.lock yet and uv sync
# cannot use constraints - once we switch to uv.lock (with the
workflow that dependabot will update it
# and constraints will be generated from it, we should be able to
simply use ``uv sync`` here
-installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./devel-common
--editable ./airflow-core"
+installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./airflow-ctl
--editable ./devel-common"
Review Comment:
```suggestion
installation_command_flags=" --editable .[${AIRFLOW_EXTRAS}]
--editable ./airflow-core --editable ./task-sdk --editable ./airflow-ctl
--editable ./devel-common"
```
--
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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
gopidesupavan commented on code in PR #48099:
URL: https://github.com/apache/airflow/pull/48099#discussion_r2019181213
##
.github/workflows/airflow-distributions-tests.yml:
##
@@ -16,10 +16,14 @@
# under the License.
Review Comment:
@bugraoz93 you can do something like this to eliminate if conditons:
Workflow call will be like this
```
with:
distribution-name: "Task SDK"
distribution-cmd-format: "prepare-task-sdk-distributions"
test-type: "task-sdk-tests"
```
Inputs will be like this
```
distribution-name:
description: "The name of the distribution to test"
required: true
type: string
distribution-cmd-format:
description: "The type of distribution to test" #eg
prepare-task-sdk-distributions
required: true
type: string
test-type:
description: "distribution test typ" #eg task-sdk-tests
required: true
type: string
```
And finally you pas all the inputs
```
- name: "Prepare Airflow ${{inputs.distribution-name}}: wheel"
env:
DISTRIBUTION_TYPE: "${{ inputs.distribution-cmd-format }}"
run: >
breeze release-management "${DISTRIBUTION_TYPE}"
--distribution-format wheel
- name: "Verify wheel packages with twine"
run: |
uv tool uninstall twine || true
uv tool install twine && twine check dist/*.whl
# Conditional steps based on the distribution name
- name: >
Run unit tests for Airflow ${{inputs.distribution-name}}:Python
${{ matrix.python-version }}
env:
PYTHON_VERSION: "${{ matrix.python-version }}"
TEST_TYPE: "${{ inputs.test-type }}"
run: >
breeze testing "${TEST_TYPE}" --python "${PYTHON_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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
kaxil commented on code in PR #48099: URL: https://github.com/apache/airflow/pull/48099#discussion_r201879 ## .github/workflows/airflow-distributions-tests.yml: ## @@ -16,10 +16,14 @@ # under the License. Review Comment: cc @potiuk if you have any suggestion on how best to handle this limitation -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
kaxil commented on code in PR #48099: URL: https://github.com/apache/airflow/pull/48099#discussion_r201879 ## .github/workflows/airflow-distributions-tests.yml: ## @@ -16,10 +16,14 @@ # under the License. Review Comment: cc @potiuk if you have any suggestion on how best to handle this limitation (or any way to bump that number for us -- if that is possible) -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on code in PR #48099: URL: https://github.com/apache/airflow/pull/48099#discussion_r2018710378 ## .github/workflows/airflow-distributions-tests.yml: ## @@ -16,10 +16,14 @@ # under the License. Review Comment: Thanks for the review, Kaxil! It is there. I first created a separate workflow for airflowctl. Then I hit the limitation of Github. The limitation is to have 20 unique workflows in CI. I merged non-core distribution tests with conditional checks. So single workflow is now running both Task SDK and Airflow CTL tests tasks. See: https://github.com/apache/airflow/blob/e79becc882e4424381d44f34013b88f1a355cc8d/.github/workflows/ci.yml#L589-L625 The CI is created with the same workflow. The only difference is that it checks the distribution_name and runs the job steps accordingly. This way, whenever there is be non-core new distribution (other than providers), we can easily add without increasing the # of workflows in the CI. https://github.com/user-attachments/assets/911c8831-c743-4f00-83af-1c367502ff5e"; /> Currently, it is impossible to add a new workflow without merging some logic into the CI. :( So what made sense to me is to merge two non-core distributions at the same workflow because their steps are the same for all breeze/CI steps. I don't think we are using the Github workflow file somewhere else. Please correct me if I am missing something -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on code in PR #48099: URL: https://github.com/apache/airflow/pull/48099#discussion_r2018710378 ## .github/workflows/airflow-distributions-tests.yml: ## @@ -16,10 +16,14 @@ # under the License. Review Comment: Thanks for the review, Kaxil! It is there. I first created a separate workflow for airflowctl. Then I hit the limitation of Github. The limitation is to have 20 unique workflows in CI. I merged non-core distribution tests with conditional checks. So single workflow is now running both Task SDK and Airflow CTL tests tasks. See: https://github.com/apache/airflow/blob/e79becc882e4424381d44f34013b88f1a355cc8d/.github/workflows/ci.yml#L589-L625 The CI is created with the same workflow. The only difference is that it checks the distribution_name and runs the job steps accordingly. This way, whenever there is be non-core new distribution (other than providers), we can easily add without increasing the # of workflows in the CI. https://github.com/user-attachments/assets/911c8831-c743-4f00-83af-1c367502ff5e"; /> Currently, it is impossible to add a new workflow without merging some logic into the CI. :( So what made sense to me is to merge two non-core distributions at the same workflow because their steps are the same for all breeze/CI steps. I don't think we are using the Github workflow file somewhere. -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
kaxil commented on code in PR #48099: URL: https://github.com/apache/airflow/pull/48099#discussion_r2018555437 ## .github/workflows/airflow-distributions-tests.yml: ## @@ -16,10 +16,14 @@ # under the License. Review Comment: This is moving Task SDK tests! we still need that file -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2759683450 ```console #56 writing layer sha256:14dafa0d80636abb67eca9d72fc94ecff7252922e63b16b496435acc6ba87b65 0.0s done #56 writing layer sha256:16d1ecdd109033d94fdd80d9f9b1f02644626a6d3d1ac2b70df8e3667427e571 0.0s done #56 writing layer sha256:1b44dfcea9b467af72573b3bd9c591ead7803d87e01e07443fbd6f08e4468d3d 0.1s done #56 ERROR: error writing layer blob: unexpected status from POST request to https://ghcr.io/v2/apache/airflow/main/ci/python3.9/blobs/uploads/: 403 Forbidden ``` Not related to the changes. My token doesn't work with this CI. ```console Error: Credentials could not be loaded, please check your action inputs: Could not load credentials from any providers ``` Not related. Seems to be the credential issue from the fork. Since I run canary, it requires more permissions. -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2759522874 > Okay, fixed that last problem too in a hacky way (static exclude). I added a small method to overcome the problem for now but I will find a way to automate that in the future for non-core distributions that don't need to be in each provider package. Let's not increase the scope, It is ready for review. :) Removed the hacky way, found the reason and solved 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2756084109 ``` Error: Credentials could not be loaded, please check your action inputs: Could not load credentials from any providers ``` [CI image checks / Publish documentation](https://github.com/apache/airflow/actions/runs/14095702697/job/39483502259?pr=48099#logs) is unrelated. ``` #56 ERROR: error writing layer blob: unexpected status from POST request to https://ghcr.io/v2/apache/airflow/main/ci/python3.9/blobs/uploads/: 403 Forbidden ``` [Additional CI image checks / Push Early Image Cache / Push CI Early:3.9 image cache](https://github.com/apache/airflow/actions/runs/14095702697/job/39482692662?pr=48099#logs) is unrelated. Static checks seem like going into a circular error where the update provider build hook forces to include the ctl into those pyproject.toml files while SDK generate datamodel hook fails due to that addition. Checking that -- 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]
Re: [PR] AIP-81 airflowctl Include CI/breeze unit-testing and distribution commands [airflow]
bugraoz93 commented on PR #48099: URL: https://github.com/apache/airflow/pull/48099#issuecomment-2756177530 Okay, fixed that last problem too in a hacky way (static exclude). I added a small method to overcome the problem for now but I will find a way to automate that in the future for non-core distributions that don't need to be in each provider package. Let's not increase the scope, It is ready for review. :) -- 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]
