Add global pull policy configuration command #1008

nkuik · 2021-01-06T16:41:35Z

Summary

Adds the following commands to set, list, or unset a global container pull policy:

pack config pull-policy (lists the configured policy--default is "always")
pack config pull-policy <if-not-present | always | never> (set the default pull-policy)
pack config pull-policy --unset (unset a configured global policy and reset to default--"always)

The changes still allow --pull-policy to be provided as an option in the command line. If --pull-policy is provided, it takes precedence to any globally configured pull-policy

Output

Before

These commands did not exist, so I can't think of any output to show.

After

`pack config -h`

$  out/pack config -h
Interact with Pack's configuration

Usage:
  pack config [command]

Available Commands:
  default-builder   List, set and unset the default builder used by other commands
  experimental      Print and set the current 'experimental' value from the config
  trusted-builders  Interact with trusted builders
  run-image-mirrors Interact with run image mirrors
  pull-policy       List, set and unset the global pull policy used by other commands
  registries        (experimental) Interact with registries

Flags:
  -h, --help   Help for 'config'

Global Flags:
      --no-color     Disable color output
  -q, --quiet        Show less output
      --timestamps   Enable timestamps in output
  -v, --verbose      Show more output

Use "pack config [command] --help" for more information about a command.

`pack config pull-policy -h`

$ out/pack config pull-policy -h
A Buildpack Registry is a (still experimental) place to publish, store, and discover buildpacks.

You can use this command to list, set, and unset the default pull policy that will be used when working with containers:
* To list your pull policy, run `pack config pull-policy`.
* To set your pull policy, run `pack config pull-policy <pull-policy>`.
* To unset your pull policy, run `pack config pull-policy --unset`.
Unsetting the pull policy will reset the policy to the default, which is always

Usage:
  pack config pull-policy <if-not-present | always | never> [flags]

Aliases:
  pull-policy, pull-policy

Flags:
  -h, --help    Help for 'pull-policy'
  -u, --unset   Unset pull policy, and set it back to the default pull-policy, which is always

Global Flags:
      --no-color     Disable color output
  -q, --quiet        Show less output
      --timestamps   Enable timestamps in output
  -v, --verbose      Show more output

`pack config pull-policy`

When no other policy has been configured.

$ out/pack config pull-policy
The current pull policy is always

`pack config pull-policy never`

When no other policy has been configured.

$ out/pack config pull-policy never
Successfully set never as the pull policy

`pack config pull-policy if-not-present`

When no other policy has been configured.

$ out/pack config pull-policy if-not-present
Successfully set if-not-present as the pull policy

`pack config pull-policy always`

When no other policy has been configured.

$ out/pack config pull-policy always
Successfully set always as the pull policy

`pack config pull-policy never`

When never was already the configured policy.

$ out/pack config pull-policy always
Pull policy is already set to never

`pack config pull-policy invalid-policy`

When never was already the configured policy.

$ out/pack config pull-policy invalid-policy
ERROR: invalid pull policy invalid-policy

`pack config pull-policy --unset`

$ out/pack config pull-policy --unset
Successfully unset pull policy never
Pull policy has been set to always

`pack config pull-policy never --unset` (invalid command)

$ out/pack config pull-policy never --unset
ERROR: pull policy and --unset cannot be specified simultaneously

Documentation

Should this change be documented?
- I would imagine, but not totally sure? see Allow global configuration of default pull-policy #991 (If it should be documented, I could do that as well)

Codecov Report

Merging #1008 (5f84486) into main (55ae0ac) will increase coverage by 0.15%.
The diff coverage is 94.60%.

@@            Coverage Diff             @@
##             main    #1008      +/-   ##
==========================================
+ Coverage   79.47%   79.61%   +0.15%     
==========================================
  Files         129      130       +1     
  Lines        5716     5777      +61     
==========================================
+ Hits         4542     4599      +57     
- Misses        755      757       +2     
- Partials      419      421       +2

Flag	Coverage Δ
os_linux	`79.56% <94.60%> (+0.15%)`	⬆️
os_macos	`76.97% <94.60%> (+0.18%)`	⬆️
os_windows	`100.00% <ø> (ø)`
unit	`79.61% <94.60%> (+0.15%)`	⬆️

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

internal/commands/build.go

internal/commands/builder_create_test.go

internal/commands/buildpack_package.go

internal/commands/config.go

internal/commands/config_pull_policy.go

nkuik · 2021-01-07T07:30:27Z

internal/commands/config_pull_policy.go

+				}
+
+				pullPolicy, err := pubcfg.ParsePullPolicy(cfg.PullPolicy)
+				if err != nil {


Don't necessarily need to check for error here, as the pull policy has been set to one (line 34 this file) that we know is valid. But there could easily be more succinct ways to do this.

Signed-off-by: Nathan Kuik <nkuik@toogoodtogo.com>

internal/commands/config_pull_policy.go

dfreilich

Overall, this is phenomenal, and really helpful. I had a bunch of nit suggestions, mostly stylistic, but this is super helpful.

One addition location to make this change is in internal/commands/package_buildpack.go
Also, one additional location that I'd add a test in is in internal/commands/confg_test.go, specifically here, just to provide assurance that the command is displayed as we would expect.

internal/commands/buildpack.go

dfreilich · 2021-01-07T09:22:19Z

internal/commands/buildpack_package.go

 			var err error
-			pullPolicy, err := pubcfg.ParsePullPolicy(flags.Policy)
+			var pullPolicy pubcfg.PullPolicy


nit: In this particular case, you should be able to remove both of these lines, and just do:

pullPolicy, err := pubcfg.ParsePullPolicy(stringPolicy)

below

internal/commands/buildpack_package.go

internal/commands/config.go

internal/commands/config_pull_policy.go

internal/commands/config_pull_policy_test.go

Signed-off-by: Nathan Kuik <nkuik@toogoodtogo.com>

dfreilich

This is super close. Just a few more small changes I'd like to see, if you have the time.

internal/commands/config.go

dfreilich · 2021-01-08T12:53:22Z

internal/commands/config_pull_policy.go

+		Args:    cobra.MaximumNArgs(1),
+		Short:   "List, set and unset the global pull policy used by other commands",
+		Aliases: []string{"pull-policy"},
+		Long: bpRegistryExplanation + "\n\nYou can use this command to list, set, and unset the default pull policy that will be used when working with containers:\n" +


This is still relevant, bpRegistryExplanataion isn't relevant for understanding pull-policies

internal/commands/config_pull_policy.go

internal/commands/config_pull_policy_test.go

internal/commands/config_pull_policy.go

Signed-off-by: Nathan Kuik <nkuik@toogoodtogo.com>

dfreilich

This should go out in the release (0.17.0), not the immediately upcoming release (0.16.0), to give us a bit more time to test it and make sure no 🐛 are slipping by us. Thanks again for the help!

nkuik force-pushed the 991-global-pull-policy-configuration branch from 7488370 to 677b1e9 Compare January 6, 2021 16:43

nkuik commented Jan 7, 2021

View reviewed changes

internal/commands/build.go Show resolved Hide resolved

nkuik commented Jan 7, 2021

View reviewed changes

internal/commands/builder_create_test.go Show resolved Hide resolved

nkuik commented Jan 7, 2021

View reviewed changes

internal/commands/buildpack_package.go Outdated Show resolved Hide resolved

nkuik commented Jan 7, 2021

View reviewed changes

internal/commands/config.go Show resolved Hide resolved

nkuik commented Jan 7, 2021

View reviewed changes

internal/commands/config_pull_policy.go Show resolved Hide resolved

nkuik commented Jan 7, 2021

View reviewed changes

Nathan Kuik added 5 commits January 7, 2021 08:31

Add PullPolicy to config

54d29db

Signed-off-by: Nathan Kuik <nkuik@toogoodtogo.com>

Add ConfigPullPolicy function and tests

dd08261

Signed-off-by: Nathan Kuik <nkuik@toogoodtogo.com>

Add ConfigPullPolicy as experimental command

cd20536

Signed-off-by: Nathan Kuik <nkuik@toogoodtogo.com>

Use configured PullPolicy if pull-policy option not specified in command

e46f6e6

Signed-off-by: Nathan Kuik <nkuik@toogoodtogo.com>

Move ConfigurePullPolicy out of experimental

6c780b2

Signed-off-by: Nathan Kuik <nkuik@toogoodtogo.com>

nkuik changed the title ~~991 global pull policy configuration~~ Global pull policy configuration Jan 7, 2021

nkuik force-pushed the 991-global-pull-policy-configuration branch from 677b1e9 to 6c780b2 Compare January 7, 2021 07:43

nkuik commented Jan 7, 2021

View reviewed changes

internal/commands/config_pull_policy.go Show resolved Hide resolved

nkuik marked this pull request as ready for review January 7, 2021 07:54

nkuik requested a review from a team as a code owner January 7, 2021 07:54

nkuik mentioned this pull request Jan 7, 2021

Allow global configuration of default pull-policy #991

Closed

1 task

dfreilich requested changes Jan 7, 2021

View reviewed changes

nkuik force-pushed the 991-global-pull-policy-configuration branch from aa61e55 to 14fb1a1 Compare January 8, 2021 08:09

Make suggested changes

82d792e

Signed-off-by: Nathan Kuik <nkuik@toogoodtogo.com>

nkuik force-pushed the 991-global-pull-policy-configuration branch from 14fb1a1 to 82d792e Compare January 8, 2021 08:10

nkuik requested a review from dfreilich January 8, 2021 09:37

dfreilich added this to the 0.17.0 milestone Jan 8, 2021

dfreilich added the type/enhancement Issue that requests a new feature or improvement. label Jan 8, 2021

dfreilich requested changes Jan 8, 2021

View reviewed changes

Some more suggestions

c62a8f3

Signed-off-by: Nathan Kuik <nkuik@toogoodtogo.com>

nkuik force-pushed the 991-global-pull-policy-configuration branch from 14fb55b to c62a8f3 Compare January 8, 2021 13:30

nkuik requested a review from dfreilich January 8, 2021 13:44

Merge branch 'main' into 991-global-pull-policy-configuration

884b74d

dfreilich approved these changes Jan 11, 2021

View reviewed changes

dfreilich changed the title ~~Global pull policy configuration~~ Add global pull policy configuration command Jan 11, 2021

dfreilich merged commit 99ada47 into buildpacks:main Jan 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add global pull policy configuration command #1008

Add global pull policy configuration command #1008

nkuik commented Jan 6, 2021

codecov bot commented Jan 6, 2021 •

edited

Loading

nkuik Jan 7, 2021

dfreilich left a comment

dfreilich Jan 7, 2021

dfreilich left a comment

dfreilich Jan 8, 2021

dfreilich left a comment

Add global pull policy configuration command #1008

Add global pull policy configuration command #1008

Conversation

nkuik commented Jan 6, 2021

Summary

Output

Before

After

pack config -h

pack config pull-policy -h

pack config pull-policy

pack config pull-policy never

pack config pull-policy if-not-present

pack config pull-policy always

pack config pull-policy never

pack config pull-policy invalid-policy

pack config pull-policy --unset

pack config pull-policy never --unset (invalid command)

Documentation

Related

codecov bot commented Jan 6, 2021 • edited Loading

Codecov Report

nkuik Jan 7, 2021

Choose a reason for hiding this comment

dfreilich left a comment

Choose a reason for hiding this comment

dfreilich Jan 7, 2021

Choose a reason for hiding this comment

dfreilich left a comment

Choose a reason for hiding this comment

dfreilich Jan 8, 2021

Choose a reason for hiding this comment

dfreilich left a comment

Choose a reason for hiding this comment

`pack config -h`

`pack config pull-policy -h`

`pack config pull-policy`

`pack config pull-policy never`

`pack config pull-policy if-not-present`

`pack config pull-policy always`

`pack config pull-policy never`

`pack config pull-policy invalid-policy`

`pack config pull-policy --unset`

`pack config pull-policy never --unset` (invalid command)

codecov bot commented Jan 6, 2021 •

edited

Loading