Conversation
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
This pull request introduces a significant optimization to the checkDockerImage function in the C2D engine. It now prioritizes checking for Docker images locally on the compute node before falling back to querying remote registries. This change can improve performance by reducing network requests and enhance reliability by leveraging pre-pulled images. The local image check also includes platform validation (architecture and OS) to ensure compatibility. Comprehensive integration tests have been added to cover local image existence, platform matching/mismatching, and the fallback mechanism.
Comments:
• [INFO][style] The normalization of amd64 to x86_64 for the architecture is a good compatibility touch. It ensures consistency when checkManifestPlatform is called, assuming it expects x86_64 for AMD64 architectures. It might be helpful to add a small comment here explaining why this normalization is done, e.g., "Normalizing architecture for checkManifestPlatform compatibility."
• [INFO][style] The localErr is typed as any. While often acceptable for caught errors, consider if a more specific type like Error or Dockerode.DockerError could be used to improve type safety and provide better context in the catch block.
• [INFO][other] It's good that cleanup of test images is considered. The current comment ((commented out to avoid breaking other tests that might use these images)) clarifies the decision. If this is a persistent concern, consider making the test images unique or ensuring tests are isolated such that cleanup here doesn't impact others. For now, acknowledging the trade-off is fine.
alexcos20
requested review from
andreip136,
bogdanfazakas,
denisiuriet,
giurgiur99 and
ndrpp
as code owners
| ? checkManifestPlatform(localPlatform, platform) | ||
| : true | ||
|
|
||
| if (isValidPlatform) { |
There was a problem hiding this comment.
Should we check remote for compatible images?
There was a problem hiding this comment.
this is only for local images. i am not sure if you can pull images for other architecture, so my guess would be if you have the image locally, it's compatible. but better to be sure and check it.
There was a problem hiding this comment.
actually, you can pull image for another arhitecture..
You can force Docker to pull a specific architecture:
docker pull --platform linux/arm64 myimage:tag2 participants
Fixes #1035
Changes proposed in this PR: