[build-images] Authenticate in both org default registries#217
Conversation
Agent-Logs-Url: https://github.com/prefapp/firestarter-workflows/sessions/6957c57f-b95e-45bd-9e82-d961ad58fb34 Co-authored-by: alambike <1025564+alambike@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the build-images workflow to authenticate against both the snapshots and releases registries so builds don’t fail when a Dockerfile pulls a base image from the “other” default registry.
Changes:
- In
BuildImages.execute(), replaced the type-conditional default-registry login with unconditional logins to bothsnapshots_registryandreleases_registry. - Updated
test_executeexpectations to account for the additional registry login call.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| firestarter/workflows/build_images/build_images.py | Logs into both default registries before building flavors to prevent cross-registry FROM pull failures. |
| firestarter/tests/test_build_images_functionality.py | Adjusts test_execute login call counts to reflect the added login. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.login_required: | ||
| self.login( | ||
| self.auth_strategy, | ||
| default_registry, | ||
| default_registry_creds, | ||
| self.snapshots_registry, | ||
| self.snapshots_registry_creds, |
There was a problem hiding this comment.
execute() still computes default_registry = getattr(self, f"{self.type}_registry") above this block, but after switching to explicit logins for snapshots/releases it’s no longer used anywhere. Consider removing default_registry (and potentially inlining/renaming default_registry_creds to reflect it’s only used for extra_registries) to avoid dead code and confusion about which registry is actually used.
| execute_function_test( | ||
| mocker, "*", checkout_calls=1, login_calls=5, compile_image_calls=1 | ||
| mocker, "*", checkout_calls=1, login_calls=6, compile_image_calls=1 | ||
| ) | ||
| execute_function_test( | ||
| mocker, "flavor1, flavor3", checkout_calls=1, login_calls=5, compile_image_calls=1 | ||
| mocker, "flavor1, flavor3", checkout_calls=1, login_calls=6, compile_image_calls=1 |
There was a problem hiding this comment.
test_execute only asserts the count of login() calls. With the new “login to both registries” behavior, this becomes more brittle and doesn’t verify the important part (that we logged into both snapshots_registry and releases_registry). Consider asserting against login_mock.call_args_list (or matching specific expected calls with ANY) so the test validates the registries/creds/auth_strategy, not just the total number of calls.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The
build-imagesworkflow only authenticated against whichever default registry matched the currenttype(snapshotsorreleases), causing failures when aDockerfilepulls a base image (FROM) hosted in the other registry.Changes
build_images.py: Inexecute(), replaced the single type-conditional registry login with explicit logins to bothsnapshots_registryandreleases_registryunconditionally.test_build_images_functionality.py: Updatedtest_executelogin_callsexpectations (+1 across all test cases) to reflect the additional login.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
dl.dagger.io/opt/hostedtoolcache/Python/3.11.15/x64/bin/python /opt/hostedtoolcache/Python/3.11.15/x64/bin/python -m pytest firestarter/tests/ -v(dns block)If you need me to access, download, or install something from one of these locations, you can either: