Skip to content

[ci] Make ecr authenticate non blocking#40305

Merged
ywang96 merged 3 commits into
mainfrom
khluu-patch-2
Apr 19, 2026
Merged

[ci] Make ecr authenticate non blocking#40305
ywang96 merged 3 commits into
mainfrom
khluu-patch-2

Conversation

@khluu
Copy link
Copy Markdown
Collaborator

@khluu khluu commented Apr 19, 2026

There are pipelines that don't configure registry to be the same as CI

Signed-off-by: Kevin H. Luu <khluu000@gmail.com>
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

Signed-off-by: Kevin H. Luu <khluu000@gmail.com>
@mergify mergify Bot added the ci/build label Apr 19, 2026
Signed-off-by: Kevin H. Luu <khluu000@gmail.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request modifies the ecr_login function in the Buildkite image build script to allow the script to continue even if the AWS ECR login commands fail. Feedback suggests replacing the || true suppression with a warning message to ensure that authentication failures are visible in the logs for debugging purposes, rather than being silently ignored.

Comment on lines +95 to +96
aws ecr-public get-login-password --region us-east-1 | docker login --username AWS --password-stdin "$REGISTRY" || true
aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin 936637512419.dkr.ecr.us-east-1.amazonaws.com || true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using || true to suppress errors from docker login is a risky practice because it masks legitimate failures (e.g., expired AWS credentials, network issues, or missing binaries) that will cause subsequent steps to fail with less descriptive error messages.

Since the goal is to make these logins non-blocking for environments where these registries are not configured, it is better to at least log a warning when the login fails. This provides visibility in the CI logs and aids in debugging if the build fails later due to authentication issues.

Suggested change
aws ecr-public get-login-password --region us-east-1 | docker login --username AWS --password-stdin "$REGISTRY" || true
aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin 936637512419.dkr.ecr.us-east-1.amazonaws.com || true
aws ecr-public get-login-password --region us-east-1 | docker login --username AWS --password-stdin "$REGISTRY" || echo "Warning: ECR Public login failed"
aws ecr get-login-password --region us-east-1 | docker login --username AWS --password-stdin 936637512419.dkr.ecr.us-east-1.amazonaws.com || echo "Warning: ECR login failed"

@ywang96 ywang96 merged commit 629d45e into main Apr 19, 2026
6 of 7 checks passed
@ywang96 ywang96 deleted the khluu-patch-2 branch April 19, 2026 22:37
bnellnm pushed a commit to neuralmagic/vllm that referenced this pull request Apr 20, 2026
Signed-off-by: Kevin H. Luu <khluu000@gmail.com>
baonudesifeizhai pushed a commit to baonudesifeizhai/vllm that referenced this pull request Apr 23, 2026
Signed-off-by: Kevin H. Luu <khluu000@gmail.com>
avinashsingh77 pushed a commit to avinashsingh77/vllm that referenced this pull request Apr 27, 2026
Signed-off-by: Kevin H. Luu <khluu000@gmail.com>
Signed-off-by: Avinash Singh <avinashsingh.rcoem@gmail.com>
Lafunamor pushed a commit to Lafunamor/vllm that referenced this pull request May 1, 2026
Signed-off-by: Kevin H. Luu <khluu000@gmail.com>
Signed-off-by: Adrian <info@zzit.ch>
mystous pushed a commit to mystous/vllm_hybrid that referenced this pull request May 10, 2026
Signed-off-by: Kevin H. Luu <khluu000@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants