-
Notifications
You must be signed in to change notification settings - Fork 2
FDN-4269: migrate all flowdocker images to ECR/flowcommerce #651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughReplaced two Dockerfile base images with AWS ECR equivalents and added a HEALTHCHECK that curls the application's internal readiness endpoint. No other build steps or exported/public declarations changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Dockerfile (1)
2-2: Consider using COPY instead of ADD.The static analysis tool flags this line because
ADDhas implicit behavior (tar auto-extraction, URL support) that isn't needed here.COPYis more explicit and is the recommended practice for simple file/directory operations.♻️ Proposed fix
-ADD . /opt/play +COPY . /opt/playBased on static analysis hints from Hadolint.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile
🧰 Additional context used
🪛 Hadolint (2.14.0)
Dockerfile
[error] 2-2: Use COPY instead of ADD for files and folders
(DL3020)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Jenkins
🔇 Additional comments (1)
Dockerfile (1)
1-1: Pin ECR image tags to specific versions instead of:latest.Using
:latesttags can lead to non-reproducible builds—different builds at different times may pull different image versions. Pin both base images to specific version tags or SHA256 digests:-FROM 479720515435.dkr.ecr.us-east-1.amazonaws.com/flowcommerce/play_builder_java17_noble:latest as builder +FROM 479720515435.dkr.ecr.us-east-1.amazonaws.com/flowcommerce/play_builder_java17_noble:<version-tag> as builder-FROM 479720515435.dkr.ecr.us-east-1.amazonaws.com/flowcommerce/play_java17:latest +FROM 479720515435.dkr.ecr.us-east-1.amazonaws.com/flowcommerce/play_java17:<version-tag>Verify that both images exist in the ECR repository and are accessible via your CI/CD authentication before merging.
|
Click here for code coverage report: https://jenkins.flo.pub/job/flowcommerce/job/registry/job/PR-651/1/scoverage-report/ |
|
Click here for code coverage report: https://jenkins.flo.pub/job/flowcommerce/job/registry/job/PR-651/2/scoverage-report/ |
Bulk migration away from flowdocker to ECR/flowcommerce
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.