ashb commented on a change in pull request #14120:
URL: https://github.com/apache/airflow/pull/14120#discussion_r577569906
##########
File path: scripts/ci/images/ci_wait_for_all_prod_images.sh
##########
@@ -15,6 +15,16 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+if [[ ${WAIT_FOR_IMAGE} != "true" ]]; then
+ # shellcheck source=scripts/in_container/_in_container_script_init.sh
+ . "$( dirname "${BASH_SOURCE[0]}" )/../libraries/_script_init.sh"
+ echo
+ echo "Not waiting for all PROD images to appear as they are built locally
in this build"
+ echo
+ push_pull_remove_images::determine_github_registry
Review comment:
I don't understand why we need this -- the `GITHUB_REGISTRY` env
variable is already set globaly in the Workflow.
##########
File path: scripts/ci/libraries/_push_pull_remove_images.sh
##########
@@ -272,74 +272,89 @@ function push_pull_remove_images::push_prod_images() {
fi
}
-# waits for an image to be available in GitHub Packages
-function push_pull_remove_images::wait_for_image_in_github_packages() {
+# waits for an image to be available in GitHub Packages. Should be run with
`set +e`
+# the buid automatically determines which registry to use based one the images
available
Review comment:
This seems like a funny optimization to do, given that GITHUB_REGISTRY
is set. Why do we need this?
##########
File path: .github/workflows/build-images-workflow-run.yml
##########
@@ -252,6 +252,9 @@ jobs:
# Run all checks
./scripts/ci/selective_ci_checks.sh
fi
+ - name: Determine GitHub Registry
+ id: determine-github-registry
+ run: ./scripts/ci/images/ci_determine_github_registry.sh
Review comment:
And then we can remove this step, and the whole new file.
##########
File path: .github/workflows/build-images-workflow-run.yml
##########
@@ -204,6 +203,7 @@ jobs:
run-tests: ${{ steps.selective-checks.outputs.run-tests }}
run-kubernetes-tests: ${{
steps.selective-checks.outputs.run-kubernetes-tests }}
image-build: ${{ steps.selective-checks.outputs.image-build }}
+ githubRegistry: ${{
steps.determine-github-registry.outputs.githubRegistry }}
Review comment:
Having a step and a file for one if is overkill -- there are already too
many files in the CI scripts folder, which makes it harder for anyone else to
understand it.
This would work wouldn't it?
```suggestion
githubRegistry: ${{ secrets.GITHUB_REGISTRY || env.GITHUB_REGISTRY }}
```
(and we keep the `GITHUB_REGISTRY` in the env L33-34, and remove the
`GITHUB_REGISTRY_SECRET` you added L197)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
