[DX-3581] revamp private/public images handling in cre system tests pipeline#21915
[DX-3581] revamp private/public images handling in cre system tests pipeline#21915
Conversation
|
✅ No conflicts with other open PRs targeting |
| fi | ||
|
|
||
| if [[ "${ecr_type}" == "public" ]]; then | ||
| printf '%s\n' "gallery.ecr.aws/${repository}:${image_tag}" |
There was a problem hiding this comment.
| printf '%s\n' "gallery.ecr.aws/${repository}:${image_tag}" | |
| printf '%s\n' "public.ecr.aws/${repository}:${image_tag}" |
Also our public images have a namespace prefix like:
public.ecr.aws/chainlink/chainlink:2.32.0
There was a problem hiding this comment.
they do, but in my mind we can pass chainlink/chainlink as repository (path). You can find a run that used that input value here: https://github.com/smartcontractkit/chainlink/actions/runs/24145193704/job/70457263811
Tofel
force-pushed
the
dx-3581-system-tests-public-image-revamp
branch
from
f73f1f9 to
db99401
Compare
product-security-plaid-production
bot
requested review from
kalverra and
skudasov
There was a problem hiding this comment.
Pull request overview
Risk Rating: MEDIUM — Changes affect how CI resolves/pulls Chainlink images for CRE system tests and can break runs if image tags/refs are altered unexpectedly.
This PR simplifies CRE system tests image selection by requiring explicit inputs (ECR type + repo path + tag) and resolving the final Chainlink image reference deterministically for either SDLC (private) or public ECR.
Changes:
- Update CRE system tests workflow inputs to require
ecr,chainlink_image_repository_path, andchainlink_image_tag, removing the “full image” pathway. - Update the integration tests workflow to pass the new inputs to
cre-system-tests.yaml. - Refactor the image resolver script and its tests to use
ECR_TYPEand the new env var names, including trimming + case normalization.
Scrupulous human review recommended (high impact / easy to break CI):
.github/scripts/resolve-chainlink-image.shinput normalization and the exact output image string formatting.- Workflow input renames and callsites (
.github/workflows/cre-system-tests.yamland.github/workflows/integration-tests.yml) to ensure no remaining callers rely on the old input contract.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/integration-tests.yml | Updates the reusable workflow call to pass ecr + repository path/tag inputs. |
| .github/workflows/cre-system-tests.yaml | Revamps workflow inputs and wires them into the image resolver step. |
| .github/scripts/resolve-chainlink-image.sh | New resolver contract: compose image from ECR_TYPE + repo path + tag (with trimming/lowercasing). |
| .github/scripts/resolve-chainlink-image_test.sh | Updates unit tests for the new resolver contract and validation paths. |
| # Normalize all input fields to lowercase for case-insensitive handling. | ||
| repository_path="$(printf '%s' "${repository_path}" | tr '[:upper:]' '[:lower:]')" | ||
| image_tag="$(printf '%s' "${image_tag}" | tr '[:upper:]' '[:lower:]')" | ||
| ecr_type="$(printf '%s' "${ecr_type}" | tr '[:upper:]' '[:lower:]')" |
There was a problem hiding this comment.
Lowercasing CHAINLINK_IMAGE_TAG can change the resolved image reference (Docker/ECR tags are case-sensitive and may legitimately contain uppercase). Consider only normalizing ECR_TYPE (and possibly AWS_REGION/repo path), while keeping the tag as provided (after trimming), or explicitly validating/enforcing lowercase tags with a clear error instead of silently transforming them.
| test_full_image_tag() { | ||
| TESTS_RUN=$((TESTS_RUN + 1)) | ||
| run_script \ | ||
| "CHAINLINK_FULL_IMAGE=public.ecr.aws/chainlink/chainlink:2.0.0" | ||
| assert_eq "${RUN_STATUS}" "0" "full image with tag exits 0" | ||
| assert_eq "${RUN_STDOUT}" "public.ecr.aws/chainlink/chainlink:2.0.0" "full image is returned to stdout" | ||
| assert_eq "${RUN_STDERR}" "" "full image success does not write stderr" | ||
| } | ||
|
|
||
| test_full_image_digest() { | ||
| TESTS_RUN=$((TESTS_RUN + 1)) | ||
| run_script \ | ||
| "CHAINLINK_FULL_IMAGE=public.ecr.aws/chainlink/chainlink@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" | ||
| assert_eq "${RUN_STATUS}" "0" "full image with digest exits 0" | ||
| assert_eq "${RUN_STDOUT}" "public.ecr.aws/chainlink/chainlink@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" "digest image is returned to stdout" | ||
| "ECR_TYPE=public" \ | ||
| "CHAINLINK_IMAGE_REPO_PATH=chainlink" \ | ||
| "CHAINLINK_IMAGE_TAG=v2.1.0" | ||
| assert_eq "${RUN_STATUS}" "0" "public ecr exits 0" | ||
| assert_eq "${RUN_STDOUT}" "public.ecr.aws/chainlink:v2.1.0" "public image is returned to stdout" | ||
| assert_eq "${RUN_STDERR}" "" "public ecr success does not write stderr" | ||
| } |
There was a problem hiding this comment.
test_full_image_tag no longer exercises "full image" handling (it tests public ECR resolution via repo path + tag). Renaming this test (and updating the main() call) would make failures easier to interpret and better reflect the current resolver behavior.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
This change simplifies Chainlink image resolution to always compose the final image from explicit inputs and ECR_TYPE:
ECR_TYPE,CHAINLINK_IMAGE_REPO,CHAINLINK_IMAGE_TAGECR_TYPEmust be one of:public→public.ecr.aws/<repository>:<image_tag>sdlc→<aws_account>.dkr.ecr.<aws_region>.amazonaws.com/<repository>:<image_tag>sdlc, bothAWS_ACCOUNT_NUMBERandAWS_REGIONevn vars are requiredInputs are trimmed and lowercased before validation/resolution.
Tests: