run/prepare/fetch: replace --no-store and --store-only with --pull-policy #3554

cgonyeo · 2017-01-20T23:16:15Z

This replaces the --no-store and --store-only flags with a singular
flag --pull-policy that can accept one of three things, never,
new, and update. This is purely a cli change, there are no under the
hood impacts. --no-store has been aliased to --pull-policy=update
and --store-only has been aliased to --pull-policy=never for
compatibility reasons.

These changes come from a discussion in #2937.

cgonyeo · 2017-01-24T21:13:51Z

Looking at the semaphore output, only the kvm run through of the tests failed, and the failing tests aren't things I would expect to be impacted by this. Anyone know if this is just flaky tests, or is there a problem with my PR?

lucab · 2017-01-25T10:59:36Z

Except for one weird kvm output, I think most other failures are real regressions introduce in this PR. For example, looking at Jenkins output shows several fetch and render failures.

cgonyeo · 2017-01-25T22:54:41Z

Looks like things are passing now. The problem appears to have been that the default behavior has changed for rkt fetch. It now defaults to --pull-policy=update, which is equivalent to --no-store, which I believe has different output when it determines that an image doesn't need to be fetched, meaning that the regex's used by the tests weren't matching. I added in a --pull-policy=new to all of the failing tests in the appropriate places, and this appears to have fixed things.

s-urbaniak

just a nit regarding the deprecation policy, else LGTM.

s-urbaniak · 2017-01-26T09:43:31Z

Documentation/image-fetching-behavior.md


 ## General Behavior

-The following table describes the meaning of the `--store-only` and `--no-store` flags.
+The following table describes the meaning of the `--pull-poloicy` flag.


--pull-policy

s-urbaniak · 2017-01-26T09:43:54Z

Documentation/subcommands/fetch.md

-This behavior can be changed by using the `--store-only` and `--no-store` flags.
-Their meanings are detailed in the [image fetching behavior][img-fetch] documentation.
+This behavior can be changed by using the `--pull-policy` flag.
+Usage of this flag is details in the [image fetching behavior][img-fetch] documentation.


"is detailed"?

s-urbaniak · 2017-01-26T09:45:06Z

Documentation/subcommands/fetch.md

 | `--signature` |  `` | A file path | Local signature file to use in validating the preceding image |
-| `--store-only` |  `false` | `true` or `false` | Use only available images in the store (do not discover or download from remote URLs). See [image fetching behavior][img-fetch] |
+| `--pull-policy` | `new` | `never`, `new`, or `update` | Sets the policy for when to fetch an image over the internet. See [image fetching behavior][img-fetch] |


I'd remove "over the internet" here and below.

s-urbaniak · 2017-01-26T09:51:59Z

rkt/fetch.go

@@ -54,6 +57,7 @@ func init() {
 	cmdFetch.Flags().BoolVar(&flagStoreOnly, "store-only", false, "use only available images in the store (do not discover or download from remote URLs)")
 	cmdFetch.Flags().BoolVar(&flagNoStore, "no-store", false, "fetch images ignoring the local store")


I think we should mark store-only and no-store explicitly as deprecated here and elsewhere.

cgonyeo · 2017-01-26T17:35:31Z

Nits should be fixed now. Changed the description of --no-store and --store-only to (deprecated) alias for --pull-policy=<policy>.

lucab · 2017-01-30T10:18:32Z

Code looks fine, but I'll take a second pass today or tomorrow regarding behavioral changes introduced in here.

lucab · 2017-01-30T17:15:37Z

It defaults to new, unless the command being called is rkt fetch, in which case it will default to update.

Just to double-check: this is to retain compatibility with previous behavior, and test failure were not strictly due to changed behavior, correct?

cgonyeo · 2017-01-30T19:03:27Z

rkt fetch defaulting to update is a different behavior than before, this PR changes it. The behavior before this PR was (with the new terminology) rkt fetch defaulting to new. This caused some tests to fail, because due to this change in behavior they needed to now pass --pull-policy=new to have the same behavior as when they were written.

cgonyeo · 2017-01-30T20:51:08Z

This behavioral change was to address half of #2937, with the other half being the recent docker2aci changes here: appc/docker2aci#237.

lucab · 2017-01-30T20:59:49Z

Thanks for the answers and sorry for the confusion, I was focused on the rework too much and actually forgot the initial request in the original ticket.

lucab · 2017-01-30T21:06:22Z

rkt/fetch.go

@@ -51,9 +54,10 @@ func init() {
 	cmdFetch.Flags().SetInterspersed(false)

 	cmdFetch.Flags().Var((*appAsc)(&rktApps), "signature", "local signature file to use in validating the preceding image")
-	cmdFetch.Flags().BoolVar(&flagStoreOnly, "store-only", false, "use only available images in the store (do not discover or download from remote URLs)")
-	cmdFetch.Flags().BoolVar(&flagNoStore, "no-store", false, "fetch images ignoring the local store")
+	cmdFetch.Flags().BoolVar(&flagStoreOnly, "store-only", false, "(deprecated) alias for --pull-policy=never")


FYI you may directly mark this as deprecated via pflags MarkDeprecated.

Oh that's super cool, I didn't know cobra had that. Changing it now.

lucab

LGTM except for a very minor UI nit.

…licy This replaces the `--no-store` and `--store-only` flags with a singular flag `--pull-policy` that can accept one of three things, `never`, `new`, and `update`. This is purely a cli change, there are no under the hood impacts. `--no-store` has been aliased to `--pull-policy=update` and `--store-only` has been aliased to `--pull-policy=never` for compatibility reasons.

cgonyeo · 2017-01-30T21:20:11Z

I've gotten LGTMs from @s-urbaniak and @lucab, and I'm happy with this, so I'll merge when the tests are green.

s-urbaniak

LGTM

cgonyeo force-pushed the pull-policy branch 2 times, most recently from b8704b4 to de68eb9 Compare January 23, 2017 23:25

lucab added this to the v1.24.0 milestone Jan 25, 2017

lucab added component/store needs/some-changes labels Jan 25, 2017

cgonyeo force-pushed the pull-policy branch 2 times, most recently from 5059a0a to 487eb2a Compare January 25, 2017 21:30

s-urbaniak suggested changes Jan 26, 2017

View reviewed changes

cgonyeo force-pushed the pull-policy branch 2 times, most recently from dd1bba0 to 50b88bd Compare January 26, 2017 17:34

lucab self-requested a review January 30, 2017 10:17

lucab reviewed Jan 30, 2017

View reviewed changes

lucab approved these changes Jan 30, 2017

View reviewed changes

cgonyeo force-pushed the pull-policy branch from 50b88bd to 0de5425 Compare January 30, 2017 21:19

s-urbaniak approved these changes Jan 31, 2017

View reviewed changes

s-urbaniak merged commit 548c7d0 into rkt:master Jan 31, 2017

cgonyeo deleted the pull-policy branch January 31, 2017 18:28

lucab mentioned this pull request Feb 1, 2017

rkt fetch doesn't ever fetch updated image if download locally #2937

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

run/prepare/fetch: replace --no-store and --store-only with --pull-policy #3554

run/prepare/fetch: replace --no-store and --store-only with --pull-policy #3554

cgonyeo commented Jan 20, 2017

cgonyeo commented Jan 24, 2017

lucab commented Jan 25, 2017

cgonyeo commented Jan 25, 2017

s-urbaniak left a comment

s-urbaniak Jan 26, 2017

s-urbaniak Jan 26, 2017

s-urbaniak Jan 26, 2017

s-urbaniak Jan 26, 2017

cgonyeo commented Jan 26, 2017

lucab commented Jan 30, 2017

lucab commented Jan 30, 2017

cgonyeo commented Jan 30, 2017

cgonyeo commented Jan 30, 2017

lucab commented Jan 30, 2017

lucab Jan 30, 2017 •

edited

Loading

cgonyeo Jan 30, 2017

lucab left a comment

cgonyeo commented Jan 30, 2017

s-urbaniak left a comment

		@@ -54,6 +57,7 @@ func init() {
		cmdFetch.Flags().BoolVar(&flagStoreOnly, "store-only", false, "use only available images in the store (do not discover or download from remote URLs)")
		cmdFetch.Flags().BoolVar(&flagNoStore, "no-store", false, "fetch images ignoring the local store")

run/prepare/fetch: replace --no-store and --store-only with --pull-policy #3554

run/prepare/fetch: replace --no-store and --store-only with --pull-policy #3554

Conversation

cgonyeo commented Jan 20, 2017

cgonyeo commented Jan 24, 2017

lucab commented Jan 25, 2017

cgonyeo commented Jan 25, 2017

s-urbaniak left a comment

Choose a reason for hiding this comment

s-urbaniak Jan 26, 2017

Choose a reason for hiding this comment

s-urbaniak Jan 26, 2017

Choose a reason for hiding this comment

s-urbaniak Jan 26, 2017

Choose a reason for hiding this comment

s-urbaniak Jan 26, 2017

Choose a reason for hiding this comment

cgonyeo commented Jan 26, 2017

lucab commented Jan 30, 2017

lucab commented Jan 30, 2017

cgonyeo commented Jan 30, 2017

cgonyeo commented Jan 30, 2017

lucab commented Jan 30, 2017

lucab Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

cgonyeo Jan 30, 2017

Choose a reason for hiding this comment

lucab left a comment

Choose a reason for hiding this comment

cgonyeo commented Jan 30, 2017

s-urbaniak left a comment

Choose a reason for hiding this comment

lucab Jan 30, 2017 •

edited

Loading