Skip to content
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

Use --pull-policy if-not-present when running pack build #373

Merged
merged 2 commits into from
Mar 7, 2022

Conversation

edmorley
Copy link
Member

@edmorley edmorley commented Mar 6, 2022

Since it:

  1. Saves ~2 seconds per integration test re-pulling an image that's already up to date.
  2. Helps prevent hitting Docker Hub or ECR rate limits from duplicate (and redundant) image pulling (that counts agains the rate limit even when it's a no-op).

Longer term, if Pack CLI supports a periodic pulling mode (buildpacks/pack#1368), we can switch to that, however for now this is the lesser of two evils - and in most cases Pack usage outside of libcnb-test will ensure that newer builder images are pulled from time to time.

See #306 for more details.

See also:
https://buildpacks.io/docs/tools/pack/cli/pack_config_pull-policy/

Fixes #306.
GUS-W-10799284.

@edmorley edmorley added libcnb-test faster tests Things that improve the runtime of tests labels Mar 6, 2022
@edmorley edmorley self-assigned this Mar 6, 2022
@joshwlewis
Copy link
Member

This probably means consumers should run a docker pull builder-image in CI prior to starting a suite of integration tests. Is there anything this library could/should do to help with that?

@edmorley
Copy link
Member Author

edmorley commented Mar 7, 2022

CI won't have any cached images (unless using the paid circle ci docker layer caching feature, which we'd never want to use with pack), so will always pull the latest image in the first test run in a job. This PR just means subsequent tests in that job don't needlessly pull again after that.

Comment on lines 79 to 81
// Adjust pull-policy to prevent redundant image-pulling, which slows CI and risks hitting registry rate limits.
String::from("--pull-policy"),
String::from("if-not-present"),
Copy link
Member

@Malax Malax Mar 7, 2022

Choose a reason for hiding this comment

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

Nit-ish: PackBuildCommand was meant to abstract away the building of the pack command from out actual logic which flags should be passed, etc.

I would love to see a pull_policy field on PackBuildCommand that is of type PullPolicy and the "logic" (even though its a static value at this point) moved to the call-site to keep the separation of concerns.

@Malax
Copy link
Member

Malax commented Mar 7, 2022

Marking this as "breaking change". Although unlikely, if users rely on automatic pulling to keep up-to-date, this will break them. I don't think this should be a huge concern for us at this point though.

Since it:
1. Saves ~2 seconds per integration test re-pulling an image that's already up to date.
2. Helps prevent hitting Docker Hub or ECR rate limits from duplicate (and redundant)
   image pulling (that counts agains the rate limit even when it's a no-op).

Longer term, if Pack CLI supports a periodic pulling mode (buildpacks/pack#1368), we
can switch to that, however for now this is the lesser of two evils - and in most cases
Pack usage outside of `libcnb-test` will ensure that newer builder images are pulled
from time to time.

Fixes #306.
@edmorley edmorley requested a review from Malax March 7, 2022 11:24
Copy link
Member

@Malax Malax left a comment

Choose a reason for hiding this comment

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

:shipit: Thanks!

@edmorley edmorley merged commit 95feb4c into main Mar 7, 2022
@edmorley edmorley deleted the edmorley/pack-pull-policy branch March 7, 2022 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change faster tests Things that improve the runtime of tests libcnb-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use --pull-policy if-not-present with Pack CLI in libcnb-test
3 participants