potiuk commented on a change in pull request #17356:
URL: https://github.com/apache/airflow/pull/17356#discussion_r682549353
##########
File path: .dockerignore
##########
@@ -40,9 +40,6 @@
!scripts/in_container
!scripts/docker
-# Add provider packages to the context
-!provider_packages
Review comment:
I can easily separate it out. It's also optimizing the image build
experience, but I can move it to separate PR.
##########
File path: .github/workflows/ci.yml
##########
@@ -1227,10 +1123,8 @@ ${{ hashFiles('.pre-commit-config.yaml') }}"
RUNS_ON: ${{ fromJson(needs.build-info.outputs.runsOn) }}
PYTHON_MAJOR_MINOR_VERSION: ${{ matrix.python-version }}
CURRENT_PYTHON_MAJOR_MINOR_VERSIONS_AS_STRING:
${{needs.build-info.outputs.pythonVersionsListAsString}}
- # Only run it for direct pushes
- if: >
- github.ref == 'refs/heads/main' || github.ref == 'refs/heads/v1-10-test'
||
- github.ref == 'refs/heads/v2-0-test' || github.ref ==
'refs/heads/v2-1-test'
+ # Only run it for direct pushes and scheduled builds
+ if: github.event_name == 'push' || github.event_name == 'schedule'
Review comment:
Not really :). I was looking on how to generalize that, but I noticed
that there is the "push" if at the top of the file:
```
push:
branches: ['main', 'v[0-9]+-[0-9]+-test']
```
##########
File path: .github/workflows/ci.yml
##########
@@ -1227,10 +1123,8 @@ ${{ hashFiles('.pre-commit-config.yaml') }}"
RUNS_ON: ${{ fromJson(needs.build-info.outputs.runsOn) }}
PYTHON_MAJOR_MINOR_VERSION: ${{ matrix.python-version }}
CURRENT_PYTHON_MAJOR_MINOR_VERSIONS_AS_STRING:
${{needs.build-info.outputs.pythonVersionsListAsString}}
- # Only run it for direct pushes
- if: >
- github.ref == 'refs/heads/main' || github.ref == 'refs/heads/v1-10-test'
||
- github.ref == 'refs/heads/v2-0-test' || github.ref ==
'refs/heads/v2-1-test'
+ # Only run it for direct pushes and scheduled builds
+ if: github.event_name == 'push' || github.event_name == 'schedule'
Review comment:
I can split it into several ones, no problem
##########
File path: .dockerignore
##########
@@ -40,9 +40,6 @@
!scripts/in_container
!scripts/docker
-# Add provider packages to the context
-!provider_packages
Review comment:
Done.
##########
File path: scripts/ci/libraries/_parallel.sh
##########
@@ -82,9 +85,9 @@ function parallel::monitor_loop() {
continue
fi
- echo "${COLOR_BLUE}### The last lines for ${parallel_process}
process: ${directory}/stdout ###${COLOR_RESET}"
+ echo "${COLOR_BLUE}### The last ${PARALLEL_TAIL_LENGTH} lines for
${parallel_process} process: ${directory}/stdout ###${COLOR_RESET}"
Review comment:
Needed to see both legacy and new image being pulled in logs
##########
File path: .github/workflows/ci.yml
##########
@@ -1103,110 +1103,6 @@ ${{ hashFiles('.pre-commit-config.yaml') }}"
path: /tmp/kind_logs_*
retention-days: 7
- push-prod-images-to-github-registry:
Review comment:
This is now merged into single job after constraints are pushed, and the
build happens using those new constraints, so we are going to have new tested
constraints always reflected in the latest cache images.
I could also potentially separate out this change I think If you think it is
too much in one PR.
##########
File path: scripts/ci/libraries/_push_pull_remove_images.sh
##########
@@ -90,7 +72,7 @@ function
push_pull_remove_images::check_and_rebuild_python_base_image_if_needed(
local dockerhub_python_version
dockerhub_python_version=$(docker run "${PYTHON_BASE_IMAGE}" python -c
'import sys; print(sys.version)')
local local_python_version
- local_python_version=$(docker run "${AIRFLOW_PYTHON_BASE_IMAGE}" python -c
'import sys; print(sys.version)')
+ local_python_version=$(docker run "${AIRFLOW_PYTHON_BASE_IMAGE}" python -c
'import sys; print(sys.version)' || true)
Review comment:
Accounts for case after `docker system prune --all`
##########
File path: scripts/ci/libraries/_push_pull_remove_images.sh
##########
@@ -102,6 +84,10 @@ function
push_pull_remove_images::check_and_rebuild_python_base_image_if_needed(
docker_v build \
--label
"org.opencontainers.image.source=https://github.com/${GITHUB_REPOSITORY}"; \
-t "${AIRFLOW_PYTHON_BASE_IMAGE}" -
+ else
+ echo
Review comment:
Better diagnostics of what's going on
##########
File path: scripts/ci/libraries/_push_pull_remove_images.sh
##########
@@ -151,8 +143,26 @@ function
push_pull_remove_images::pull_ci_images_if_needed() {
fi
fi
if [[ "${DOCKER_CACHE}" == "pulled" ]]; then
+ set +e
push_pull_remove_images::pull_image_if_not_present_or_forced \
"${AIRFLOW_CI_IMAGE}:${GITHUB_REGISTRY_PULL_IMAGE_TAG}"
+ local res="$?"
+ set -e
+ if [[ ${res} != "0" ]]; then
+ if [[ ${GITHUB_REGISTRY_PULL_IMAGE_TAG} == "latest" ]] ; then
+ echo
+ echo "The CI image cache does not exist. This is likely the
first time you build the image"
+ echo "Switching to 'local' cache for docker images"
+ echo
+ DOCKER_CACHE="local"
Review comment:
This is helpful if we have new python image not yet available in
gitHubRegistry (next time - python 3.10)
--
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]
