update import-image cmd to use external versions #20333

juanvallejo · 2018-07-16T21:53:36Z

juanvallejo · 2018-07-17T19:08:32Z

pkg/oc/cli/importimage/importimage.go

@@ -384,39 +413,55 @@ func (o *ImportImageOptions) importTag(stream *imageapi.ImageStream) (*imageapi.
 		existing.Generation = &zero

 	}
-	stream.Spec.Tags[tag] = *existing


@deads2k imageapiv1 switches an imagestream's spec.tags field from a map[string]TagReference to a slice of TagReferences. Not sure how to handle tag lookups in the command

@deads2k imageapiv1 switches an imagestream's spec.tags field from a map[string]TagReference to a slice of TagReferences. Not sure how to handle tag lookups in the command

You can't just iterate over the list? There shouldn't be any conflicting names.

I did not want to assume that a ReferenceTag.Name value was the same as the value used for a key in the map that was used in the internal api. Latest changes to this PR make use of the imageapi.SplitImageStreamTag helper:

origin/pkg/image/apis/image/helper.go

Lines 161 to 171 in 6f8c120

func SplitImageStreamTag(nameAndTag string) (name string, tag string, ok bool) {

parts := strings.SplitN(nameAndTag, ":", 2)

name = parts[0]

if len(parts) > 1 {

tag = parts[1]

}

if len(tag) == 0 {

tag = DefaultImageTag

}

return name, tag, len(parts) == 2

}

juanvallejo · 2018-07-17T19:17:59Z

pkg/oc/cli/importimage/importimage.go

 	isi, streamInsecure := o.newImageStreamImport(stream)
 	for tag, from := range tags {
 		insecure := streamInsecure
 		scheduled := o.Scheduled
-		oldTag, ok := stream.Spec.Tags[tag]


cc @bparees
External image api does not allow maps, and instead stores tags as a slice. Is there any preference here as to how tag lookups should occur?

nope, iterating seems reasonable to me.

juanvallejo · 2018-07-20T19:16:54Z

pkg/oc/cli/importimage/importimage.go

+		oldTagFound := false
+		var oldTag imageapiv1.TagReference
+		for _, t := range stream.Spec.Tags {
+			if t.Name == tag {


@mfojtik is there a helper I can use here to match a TagReference? I'm pretty sure matching on t.Name is not correct

juanvallejo · 2018-07-23T19:21:14Z

Switched from using TagReference.Name to using imageapi.SplitImageStreamTag(TagReference.Name) when matching to see if a user-provided tag exists, in order to mimic tag-matching logic from internal api

@bparees @mfojtik would appreciate one last look at these changes. Particularly here, here, here, and here

bparees · 2018-07-23T21:54:13Z

lgtm

soltysh · 2018-07-27T11:00:30Z

pkg/oc/cli/importimage/importimage.go

 	"k8s.io/kubernetes/pkg/api/legacyscheme"
-	kapi "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
 	kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions"

 	imageapiv1 "github.com/openshift/api/image/v1"


soltysh · 2018-07-27T11:00:37Z

pkg/oc/cli/importimage/importimage.go

 	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
 	kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions"

 	imageapiv1 "github.com/openshift/api/image/v1"
+	imageclientv1 "github.com/openshift/client-go/image/clientset/versioned/typed/image/v1"


imagev1client

soltysh · 2018-07-27T11:00:46Z

pkg/oc/cli/importimage/importimage.go

 	"github.com/openshift/origin/pkg/oc/cli/tag"
 	"github.com/openshift/origin/pkg/oc/lib/describe"
+	"k8s.io/kubernetes/pkg/kubectl/genericclioptions/printers"


Up to k8s imports

soltysh · 2018-07-27T11:02:45Z

pkg/oc/cli/importimage/importimage.go

+// an import.
+func (o *ImportImageOptions) Validate(cmd *cobra.Command) error {
+	if len(o.Target) == 0 {
+		return kcmdutil.UsageErrorf(cmd, "you must specify the name of an image stream")


Validate only works with *Options and does not accept any input, return fmt.Errorf here.

soltysh · 2018-07-27T11:04:45Z

pkg/oc/cli/importimage/importimage.go


 	if result.Status.Import != nil {
+		// TODO: remove once we have external describers
+		var internalResult imageapi.ImageStreamImport
+		if legacyscheme.Scheme.Convert(result, &internalResult, nil); err != nil {


Don't use legacyscheme it's legacy for a reason. Invoke the actual conversion function from image package, like we're doing that already elsewhere (eg. #20343).

soltysh · 2018-07-27T11:05:17Z

pkg/oc/cli/importimage/importimage.go

-			info, err := describe.DescribeImage(image.Image, imageapi.JoinImageStreamTag(stream.Name, image.Tag))
+			// TODO: remove once we have external describers
+			var internalImage imageapi.Image
+			if legacyscheme.Scheme.Convert(image.Image, &internalImage, nil); err != nil {


soltysh · 2018-07-27T11:07:32Z

pkg/oc/cli/importimage/importimage.go

+
+		oldTagFound := false
+		var oldTag imageapiv1.TagReference
+		for _, t := range stream.Spec.Tags {


You're iterating over this second time so far, create a helper and use in both places.

soltysh · 2018-07-27T11:10:26Z

pkg/oc/cli/importimage/importimage.go

 	from := o.From
 	tag := o.Tag
 	// follow any referential tags to the destination
-	finalTag, existing, multiple, err := imageapi.FollowTagReference(stream, tag)
+	finalTag, existing, multiple, err := followTagReferenceV1(stream, tag)


I hate to say that, but create a shim, and don't copy. At some point in time some victim (in my unclear memory I recall I volunteered) will fix the controller to use the externals as well and we'll just remove the shim.

I am having the same problem here, although the helper pkg/image/apis/image/v1.Convert_v1_ImageStream_To_image_ImageStream does not depend on a conversion scope, on of the conversion funcs that it depends on does: https://github.com/openshift/origin/blob/master/pkg/image/apis/image/v1/conversion.go#L160

soltysh · 2018-07-27T11:10:51Z

pkg/oc/cli/importimage/importimage_test.go

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	kapi "k8s.io/kubernetes/pkg/apis/core"
+	//kapi "k8s.io/kubernetes/pkg/apis/core"


Remove if not needed.

soltysh · 2018-07-27T11:11:28Z

pkg/oc/cli/importimage/importimage_test.go

@@ -621,7 +621,7 @@ func TestCreateImageImport(t *testing.T) {
 	}
 }

-func TestWasError(t *testing.T) {
+func CANCELTestWasError(t *testing.T) {


:) Forgot to remove - couldn't remember to use hack/test-go.sh ... -run=...

soltysh

/lgtm
/approve

openshift-ci-robot · 2018-08-03T16:31:52Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: juanvallejo, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~contrib/completions/OWNERS~~ [juanvallejo,soltysh]
hack/OWNERS
~~pkg/oc/OWNERS~~ [soltysh]
~~test/cmd/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

soltysh · 2018-08-03T19:20:20Z

/retest

openshift-ci-robot requested review from enj and soltysh July 16, 2018 21:53

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 16, 2018

juanvallejo commented Jul 17, 2018

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2018

juanvallejo commented Jul 20, 2018

View reviewed changes

juanvallejo force-pushed the jvallejo/update-importimage-to-externals branch from f33b464 to e17103b Compare July 20, 2018 19:18

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2018

juanvallejo force-pushed the jvallejo/update-importimage-to-externals branch from e17103b to 3771854 Compare July 23, 2018 19:15

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 24, 2018

soltysh suggested changes Jul 27, 2018

View reviewed changes

soltysh self-assigned this Jul 27, 2018

juanvallejo force-pushed the jvallejo/update-importimage-to-externals branch 6 times, most recently from c945dcd to add54e8 Compare August 3, 2018 02:17

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2018

juanvallejo force-pushed the jvallejo/update-importimage-to-externals branch 2 times, most recently from 16e02e5 to 9ab92a3 Compare August 3, 2018 02:52

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2018

update import-image cmd to use external versions

46f4f45

juanvallejo force-pushed the jvallejo/update-importimage-to-externals branch from 9ab92a3 to 46f4f45 Compare August 3, 2018 16:20

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2018

soltysh approved these changes Aug 3, 2018

View reviewed changes

soltysh added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2018

openshift-merge-robot merged commit 2fa8042 into openshift:master Aug 3, 2018

juanvallejo deleted the jvallejo/update-importimage-to-externals branch August 3, 2018 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update import-image cmd to use external versions #20333

update import-image cmd to use external versions #20333

juanvallejo commented Jul 16, 2018

juanvallejo Jul 17, 2018

deads2k Jul 23, 2018

juanvallejo Jul 23, 2018 •

edited

Loading

juanvallejo Jul 17, 2018

bparees Jul 17, 2018

juanvallejo Jul 20, 2018

juanvallejo commented Jul 23, 2018 •

edited

Loading

bparees commented Jul 23, 2018

soltysh Jul 27, 2018

soltysh Jul 27, 2018

soltysh Jul 27, 2018

soltysh Jul 27, 2018

soltysh Jul 27, 2018

soltysh Jul 27, 2018

soltysh Jul 27, 2018

soltysh Jul 27, 2018

juanvallejo Aug 2, 2018

soltysh Jul 27, 2018

soltysh Jul 27, 2018

juanvallejo Aug 2, 2018

soltysh left a comment

openshift-ci-robot commented Aug 3, 2018

soltysh commented Aug 3, 2018

	func SplitImageStreamTag(nameAndTag string) (name string, tag string, ok bool) {
	parts := strings.SplitN(nameAndTag, ":", 2)
	name = parts[0]
	if len(parts) > 1 {
	tag = parts[1]
	}
	if len(tag) == 0 {
	tag = DefaultImageTag
	}
	return name, tag, len(parts) == 2
	}

update import-image cmd to use external versions #20333

update import-image cmd to use external versions #20333

Conversation

juanvallejo commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo Jul 23, 2018 • edited Loading

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

juanvallejo commented Jul 23, 2018 • edited Loading

bparees commented Jul 23, 2018

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

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 3, 2018

soltysh commented Aug 3, 2018

juanvallejo Jul 23, 2018 •

edited

Loading

juanvallejo commented Jul 23, 2018 •

edited

Loading