[GitHub] [airflow] ashb commented on a change in pull request #14120: Easy switching between GitHub Container Registries

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org