Add pull-policy flag to build/create-builder/package-buildpack/rebase commands #773

dfreilich · 2020-08-04T02:06:36Z

Summary

This deprecates the --no-pull flag, in favor of --pull-policy, which as three options at the moment:

always → Always pulls the images necessary (the default behavior of pack)
never → Never pulls the images necessary (equivalent to --no-pull)
if-not-present → Pulls the image only if it is not present

It also adds the possibility for future policies to be added, by centralizing the PullPolicys.

Output

`$ pack build`

Before

After

`$ pack create-builder`

Before

After

`$ pack package-buildpack`

Before

After

`$ pack rebase`

Before

After

Documentation

Should this change be documented?
- Yes, and it will be automatically through the generated pack documentation

Codecov Report

Merging #773 into main will increase coverage by 0.42%.
The diff coverage is 99.29%.

@@            Coverage Diff             @@
##             main     #773      +/-   ##
==========================================
+ Coverage   75.59%   76.00%   +0.42%     
==========================================
  Files          77       78       +1     
  Lines        5197     5290      +93     
==========================================
+ Hits         3928     4020      +92     
- Misses        972      973       +1     
  Partials      297      297

Flag	Coverage Δ
#os_linux	`78.33% <99.13%> (+0.34%)`	⬆️
#os_macos	`74.23% <91.23%> (+0.29%)`	⬆️
#os_windows	`74.32% <99.29%> (+0.45%)`	⬆️
#unit	`76.00% <99.29%> (+0.42%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

dfreilich

Should we add a --pull-policy flag to pack inspect-builder and pack inspect-image?

dfreilich · 2020-08-05T01:24:14Z

build.go

@@ -92,7 +94,7 @@ func (c *Client) Build(ctx context.Context, opts BuildOptions) error {
 		return errors.Wrapf(err, "invalid builder '%s'", opts.Builder)
 	}

-	rawBuilderImage, err := c.imageFetcher.Fetch(ctx, builderRef.Name(), true, !opts.NoPull)
+	rawBuilderImage, err := c.imageFetcher.Fetch(ctx, builderRef.Name(), true, opts.PullPolicy)


From a code readability standpoint, I appreciate the removal of double negatives (from here, and many other place throughout the code).

dfreilich · 2020-08-05T01:27:01Z

config/pull_policy.go

+
+const (
+	// PullAlways images, even if they are present
+	PullAlways PullPolicy = iota


iota is useful for defining enums. For more on iotas, see https://github.com/golang/go/wiki/Iota or https://splice.com/blog/iota-elegant-constants-golang/

On the other hand, I also debated just defining these as strings, which would've obviated the need for a secondary String() method. If people would rather change it to a string type to make it simpler, I'm happy to do that.

config/pull_policy.go

dfreilich · 2020-08-05T01:33:34Z

create_builder.go

@@ -64,7 +66,7 @@ func (c *Client) validateRunImageConfig(ctx context.Context, opts CreateBuilderO
 	var runImages []imgutil.Image
 	for _, i := range append([]string{opts.Config.Stack.RunImage}, opts.Config.Stack.RunImageMirrors...) {
 		if !opts.Publish {
-			img, err := c.imageFetcher.Fetch(ctx, i, true, false)
+			img, err := c.imageFetcher.Fetch(ctx, i, true, opts.PullPolicy)


There were times where we had ignored the --no-pull option, which I think we shouldn't do, once we explicitly ask people what they want to do.

dfreilich · 2020-08-05T01:36:29Z

internal/commands/build.go

 }

-func validateBuildFlags(flags BuildFlags, logger logging.Logger, cfg config.Config, packClient PackClient) error {
+func validateBuildFlags(flags *BuildFlags, cfg config.Config, packClient PackClient, logger logging.Logger) error {


From a readability perspective, I found it confusing for the logger to be so early in the parameters, so I took the liberty of moving it to the end of the parameter list

jromero

This is great! Nice clean implementation! 💯

* Adds temporary method NewFetch, to allow for gradual refactoring in adding PullPolicy Signed-off-by: David Freilich <dfreilich@vmware.com>