chore(ci): Use DOCKER_REGISTRY, and compat
#17051
Conversation
This is so the registry is always postfixed with `/docker`, unless you set the `DOCKER_REGISTRY` build-arg (never done in CI, but used locally)
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
scripts/ci/Dockerfile (1)
3-6: Consider improving the clarity of registry path manipulationWhile the implementation is correct, the string manipulation
${DOCKER_ECR_REGISTRY%/docker}might not be immediately clear to all developers.Consider either:
- Adding a comment explaining the
%/dockeroperation:ARG DOCKER_ECR_REGISTRY=public.ecr.aws/docker +# Remove '/docker' suffix from DOCKER_ECR_REGISTRY to maintain compatibility ARG DOCKER_IMAGE_REGISTRY=${DOCKER_ECR_REGISTRY%/docker}
- Or using a more explicit approach:
ARG DOCKER_ECR_REGISTRY=public.ecr.aws/docker -ARG DOCKER_IMAGE_REGISTRY=${DOCKER_ECR_REGISTRY%/docker} +ARG DOCKER_IMAGE_REGISTRY=public.ecr.aws
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
scripts/ci/Dockerfile(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
scripts/ci/Dockerfile (1)
Pattern scripts/**/*: "Confirm that the code adheres to the following:
- Script efficiency, readability, and maintainability.
- Usage of environmental variables and configuration files for flexibility.
- Integration with NX workspace utilities and commands."
🔇 Additional comments (1)
scripts/ci/Dockerfile (1)
3-6: Verify documentation and build script updates
The registry changes require verification of related documentation and build scripts to ensure a smooth local development experience.
✅ Verification successful
Registry changes are properly handled across build scripts
The registry changes in the Dockerfile are well-integrated with the build system:
- The new registry variables are properly referenced in the CI scripts
- Default fallbacks are in place in
_common.shfor backward compatibility - Build scripts use the registry variables consistently
- Local development is supported through the
DOCKER_REGISTRYenvironment variable with proper defaults
The only affected files are build-related scripts, and they handle the changes appropriately. No documentation updates are required as the changes are transparent to users due to the backward compatibility layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if documentation and build scripts are updated for the new registry variables
# Look for related documentation
echo "Checking documentation..."
rg -l "DOCKER_IMAGE_REGISTRY|DOCKER_ECR_REGISTRY|DOCKER_REGISTRY" "*.md" "docs/"
# Check build scripts
echo "Checking build scripts..."
rg -l "DOCKER_IMAGE_REGISTRY|DOCKER_ECR_REGISTRY|DOCKER_REGISTRY" "scripts/"
Length of output: 492
Script:
#!/bin/bash
# Let's check the content of these files to understand the usage and potential impact
echo "=== Checking docker-login-ecr.sh ==="
cat scripts/ci/docker-login-ecr.sh
echo -e "\n=== Checking _common.sh ==="
cat scripts/ci/_common.sh
echo -e "\n=== Checking _docker.sh ==="
cat scripts/ci/_docker.sh
echo -e "\n=== Checking Dockerfile.proxy ==="
cat scripts/Dockerfile.proxy
Length of output: 3880
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17051 +/- ##
=======================================
Coverage 35.69% 35.69%
=======================================
Files 6939 6939
Lines 147081 147081
Branches 41832 41832
=======================================
+ Hits 52495 52498 +3
+ Misses 94586 94583 -3
Flags with carried forward coverage won't be shown. Click here to find out more. see 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 5 Total Test Services: 0 Failed, 5 Passed Test Services
|
When developing locally, setting the correct registry is annoying. With this we can build locally more easily with
--build-arg DOCKER_REGISTRY=docker.iowhen needed.Summary by CodeRabbit