-
Notifications
You must be signed in to change notification settings - Fork 731
build(sam): pull SAM builds in install phase to avoid timeouts #6179
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
|
/runIntegrationTests |
| // vscodeMinimum: '1.67.0', | ||
| // }, | ||
|
|
||
| generateScenario('nodejs', '18.x'), |
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.
A benefit of the old code is that it was "flat" data that did not require inspecting any logic. Copy-pasting can be very costly for logic, but for data, especially data contained in one place (this file) it can be a useful tradeoff.
However, your new code is certainly easier to read at a glance.
| # Ensure that "docker" group has permissions to the socket. | ||
| # - chown codebuild-user /var/run/docker.sock | ||
| - chmod 666 /var/run/docker.sock | ||
| # Pull Docker Images for SAM tests |
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.
Great idea!!
|
Integ tests on master usually take 25 min, whereas this PR took 35 min. However, on b3823d4 the Integ tests took 35 min, so perhaps this is variable. Since this will help with reliability, let's merge it and see how it performs over time. |
|
If the tests themselves are pulling the images when we run them, is there any reason pulling them ahead of time would increase the time of the tests? |
|
The times I mentioned are total CI time per-job, which includes the "install" phase. |
## Problem - The SAM tests are flaky, and sometimes fail CI. - SAM file scenarios section includes a ton of copy-pasting. ## Solution The SAM build images can be pulled ahead of time to reduce risk of timeout. To see this working, see the integ test logs below, and notice in the logs that `Fetching public.ecr.aws/sam/build-... Docker container image...`, resolved immediately (likely cached from pre-fetching) whereas before this took 60-90 seconds sometimes. To reduce copy pasting, establish useful defaults, and generate the scenarios based on a few args to avoid copy pasting information.
Problem
Solution
The SAM build images can be pulled ahead of time to reduce risk of timeout.
To see this working, see the integ test logs below, and notice in the logs that
Fetching public.ecr.aws/sam/build-... Docker container image..., resolved immediately (likely cached from pre-fetching) whereas before this took 60-90 seconds sometimes.To reduce copy pasting, establish useful defaults, and generate the scenarios based on a few args to avoid copy pasting information.
feature/xbranches will not be squash-merged at release time.License: I confirm that my contribution is made under the terms of the Apache 2.0 license.