Bug 1420976 - Support passing reference-policy in import-image command #13339

soltysh · 2017-03-10T08:47:25Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1420976

@mfojtik @miminar ptal

soltysh · 2017-03-10T08:47:32Z

[test]

soltysh · 2017-03-10T08:48:48Z

@openshift/api-review for the api change, ReferencePolicy in ImageImportSpec

soltysh · 2017-03-10T10:40:02Z

Forgot about updating completions.

smarterclayton · 2017-03-10T14:50:26Z

API change approved

soltysh · 2017-03-10T15:08:41Z

I've rebased and re-pushed. I can't reproduce that pkg/api/serialization_test.go failure locally :/

smarterclayton · 2017-03-10T19:53:47Z

pkg/cmd/cli/cmd/importimage.go

@@ -52,6 +52,7 @@ func NewCmdImportImage(fullName string, f *clientcmd.Factory, out, errout io.Wri
 	cmd.Flags().StringVar(&opts.From, "from", "", "A Docker image repository to import images from")
 	cmd.Flags().BoolVar(&opts.Confirm, "confirm", false, "If true, allow the image stream import location to be set or changed")
 	cmd.Flags().BoolVar(&opts.All, "all", false, "If true, import all tags from the provided source on creation or if --from is specified")
+	cmd.Flags().StringVar(&opts.ReferencePolicy, "reference-policy", sourceReferencePolicy, "Allow to request pullthrough for external image when set to 'local'. Defaults to 'source'.")


These should be constants, i.e. Local and Source

They are in pkg/cmd/cli/cmd/tag.go:79.

smarterclayton · 2017-03-10T19:54:43Z

pkg/image/api/v1/types.go

 	// IncludeManifest determines if the manifest for each image is returned in the response
-	IncludeManifest bool `json:"includeManifest,omitempty" protobuf:"varint,4,opt,name=includeManifest"`
+	IncludeManifest bool `json:"includeManifest,omitempty" protobuf:"varint,5,opt,name=includeManifest"`


Yo can't do this - you just broke the protobuf serialization. Never change the tag number on a field.

Oooops... my bad. Fixing right away.

soltysh · 2017-03-13T10:11:44Z

Comments addressed.

mfojtik · 2017-03-13T10:14:48Z

pkg/cmd/cli/cmd/importimage.go

@@ -118,6 +123,10 @@ func (o *ImportImageOptions) Validate(cmd *cobra.Command) error {
 		return kcmdutil.UsageError(cmd, "you must specify the name of an image stream")
 	}

+	if len(o.ReferencePolicy) > 0 && o.All {


open an issue about adding support for this in the future

How would you wanted to work? It's not a problem to support this right away. The thing is that all tags will get the same reference policy, including overwriting current ones.

The thing is that all tags will get the same reference policy, including overwriting current ones.

@soltysh I believe that's how it's supposed to work. Can you make it happen in this PR?

Sure, that's not a problem. Will update.

mfojtik · 2017-03-13T10:17:14Z

pkg/cmd/cli/cmd/importimage.go

@@ -440,16 +463,13 @@ func (o *ImportImageOptions) newImageStream() (*imageapi.ImageStream, *imageapi.
 							Kind: "DockerImage",
 							Name: from,
 						},
+						ReferencePolicy: imageapi.TagReferencePolicy{


i guess this should not be set at all if the reference policy is not set...

i would check below this and generate the ReferencePolicy only if len(referencePolicy) > 0

mfojtik · 2017-03-13T10:17:24Z

pkg/cmd/cli/cmd/importimage.go

@@ -498,6 +525,9 @@ func (o *ImportImageOptions) newImageStreamImportTags(stream *imageapi.ImageStre
 			},
 			To:           &kapi.LocalObjectReference{Name: tag},
 			ImportPolicy: imageapi.TagImportPolicy{Insecure: insecure},
+			ReferencePolicy: imageapi.TagReferencePolicy{


same comment as above

mfojtik · 2017-03-13T10:18:09Z

pkg/cmd/cli/cmd/importimage_test.go

@@ -338,6 +339,28 @@ func TestCreateImageImport(t *testing.T) {
 				ImportPolicy: imageapi.TagImportPolicy{Insecure: false},
 			}},
 		},
+		"import tag setting referencePolicy": {


maybe add test with invalid reference policy? (optional ;)

On second thought, I don't check Validate anywhere, so I'll drop the idea ;)

soltysh · 2017-03-13T11:37:46Z

The empty tag reference comments addressed. Rest is either under discussion or will not be fixed.

soltysh · 2017-03-14T09:42:27Z

@miminar comment addressed.

miminar

LGTM

openshift-bot · 2017-03-14T11:37:21Z

Evaluated for origin test up to 26e9be3

openshift-bot · 2017-03-14T13:00:37Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/182/) (Base Commit: f617108)

soltysh · 2017-03-14T13:07:04Z

[merge]

soltysh · 2017-03-14T19:07:22Z

Flake #13386

[merge]

soltysh · 2017-03-15T05:34:59Z

Looks like #11016.

[merge]

openshift-bot · 2017-03-15T05:37:30Z

Evaluated for origin merge up to 26e9be3

openshift-bot · 2017-03-15T05:37:39Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/90/) (Base Commit: 41a0dbd) (Image: devenv-rhel7_6070)

smarterclayton · 2017-03-15T13:06:23Z

Some examples of 110016 are image pulls - I found that out yesterday with the router. We need to start doing image prepulling and simplify the number of images we use On Mar 15, 2017, at 2:54 AM, OpenShift Bot <[email protected]> wrote: Merged #13339 <#13339>. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#13339 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pw-1KPsuuqFcQq1xLIfGLR7tlBPwks5rl4s7gaJpZM4MZHlf> .

soltysh · 2017-03-15T13:31:27Z

We need to start doing image prepulling and simplify the number of images we use

When and where?

smarterclayton · 2017-03-15T15:21:22Z

Soon and in the jobs (somehow).

…

On Wed, Mar 15, 2017 at 9:31 AM, Maciej Szulik ***@***.***> wrote: We need to start doing image prepulling and simplify the number of images we use When and where? — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#13339 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pzzPs5AQUUrJIDXpqBYDK9bP0Hljks5rl-gwgaJpZM4MZHlf> .

soltysh added the needs-api-review label Mar 10, 2017

soltysh added the component/image label Mar 10, 2017

soltysh force-pushed the bug1420976 branch from f8d682e to 0a88d02 Compare March 10, 2017 10:39

soltysh force-pushed the bug1420976 branch from 0a88d02 to 2c1674c Compare March 10, 2017 13:05

soltysh added api-approved and removed needs-api-review labels Mar 10, 2017

soltysh force-pushed the bug1420976 branch from 2c1674c to 471845c Compare March 10, 2017 15:08

smarterclayton reviewed Mar 10, 2017

View reviewed changes

soltysh force-pushed the bug1420976 branch from 471845c to b9916ba Compare March 13, 2017 10:11

mfojtik reviewed Mar 13, 2017

View reviewed changes

soltysh force-pushed the bug1420976 branch from b9916ba to 0b28445 Compare March 13, 2017 11:37

Bug 1420976 - Support passing reference-policy in import-image command

aa80c85

soltysh force-pushed the bug1420976 branch from 0b28445 to e26dfcc Compare March 14, 2017 09:41

miminar approved these changes Mar 14, 2017

View reviewed changes

Generated changes for image import reference-policy

26e9be3

soltysh force-pushed the bug1420976 branch from e26dfcc to 26e9be3 Compare March 14, 2017 11:34

openshift-bot merged commit 0009bfa into openshift:master Mar 15, 2017

soltysh deleted the bug1420976 branch March 15, 2017 09:32

Bug 1420976 - Support passing reference-policy in import-image command #13339

Bug 1420976 - Support passing reference-policy in import-image command #13339

Conversation

soltysh commented Mar 10, 2017

soltysh commented Mar 10, 2017

soltysh commented Mar 10, 2017

soltysh commented Mar 10, 2017

smarterclayton commented Mar 10, 2017

soltysh commented Mar 10, 2017

Choose a reason for hiding this comment

soltysh Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

soltysh commented Mar 13, 2017

soltysh commented Mar 14, 2017

miminar left a comment

Choose a reason for hiding this comment

openshift-bot commented Mar 14, 2017

openshift-bot commented Mar 14, 2017

soltysh commented Mar 14, 2017

soltysh commented Mar 14, 2017

soltysh commented Mar 15, 2017

openshift-bot commented Mar 15, 2017

openshift-bot commented Mar 15, 2017 • edited Loading

smarterclayton commented Mar 15, 2017 via email

soltysh commented Mar 15, 2017

smarterclayton commented Mar 15, 2017 via email

soltysh Mar 13, 2017 •

edited

Loading

soltysh Mar 13, 2017 •

edited

Loading

openshift-bot commented Mar 15, 2017 •

edited

Loading