Fix import-image --all

pkg/cmd/cli/cmd/importimage.go

@@ -129,12 +129,13 @@ func (o *ImportImageOptions) Validate(cmd *cobra.Command) error {
 
 // Run contains all the necessary functionality for the OpenShift cli import-image command.
 func (o *ImportImageOptions) Run() error {
+	// TODO: add dry-run
 	stream, isi, err := o.createImageImport()
 	if err != nil {
 		return err
 	}
 
-	// TODO: add dry-run
+	// Attempt the new, direct import path
 	result, err := o.isClient.Import(isi)
 	switch {
 	case err == client.ErrImageStreamImportUnsupported:
@@ -248,128 +249,172 @@ func (o *ImportImageOptions) waitForImport(resourceVersion string) (*imageapi.Im
 }
 
 func (o *ImportImageOptions) createImageImport() (*imageapi.ImageStream, *imageapi.ImageStreamImport, error) {
+	var isi *imageapi.ImageStreamImport
 	stream, err := o.isClient.Get(o.Name)
-	from := o.From
-	tag := o.Tag
+	// no stream, try creating one
 	if err != nil {
 		if !errors.IsNotFound(err) {
 			return nil, nil, err
 		}
-
-		// the stream is new
 		if !o.Confirm {
 			return nil, nil, fmt.Errorf("no image stream named %q exists, pass --confirm to create and import", o.Name)
 		}
-		if len(from) == 0 {
-			from = o.Target
-		}
-		if o.All {
-			stream = &imageapi.ImageStream{
-				ObjectMeta: kapi.ObjectMeta{Name: o.Name},
-				Spec:       imageapi.ImageStreamSpec{DockerImageRepository: from},
-			}
-		} else {
-			stream = &imageapi.ImageStream{
-				ObjectMeta: kapi.ObjectMeta{Name: o.Name},
-				Spec: imageapi.ImageStreamSpec{
-					Tags: map[string]imageapi.TagReference{
-						tag: {
-							From: &kapi.ObjectReference{
-								Kind: "DockerImage",
-								Name: from,
-							},
-						},
-					},
-				},
-			}
-		}
+		stream, isi = o.newImageStream()
+		return stream, isi, nil
+	}
 
+	// the stream already exists
+	if len(stream.Spec.DockerImageRepository) == 0 && len(stream.Spec.Tags) == 0 {
+		return nil, nil, fmt.Errorf("image stream does not have valid docker images to be imported")
+	}
+
+	if o.All {
+		// importing the entire repository
+		isi, err = o.importAll(stream)
+		if err != nil {
+			return nil, nil, err
+		}
 	} else {
-		// the stream already exists
-		if len(stream.Spec.DockerImageRepository) == 0 && len(stream.Spec.Tags) == 0 {
-			return nil, nil, fmt.Errorf("image stream has not defined anything to import")
+		// importing a single tag
+		isi, err = o.importTag(stream)
+		if err != nil {
+			return nil, nil, err
 		}
+	}
 
-		if o.All {
-			// importing a whole repository
-			// TODO soltysh: write tests to cover all the possible usecases!!!
-			if len(from) == 0 {
-				if len(stream.Spec.DockerImageRepository) == 0 {
-					// FIXME soltysh:
-					return nil, nil, fmt.Errorf("flag --all is applicable only to images with spec.dockerImageRepository defined")
+	return stream, isi, nil
+}
+
+func (o *ImportImageOptions) importAll(stream *imageapi.ImageStream) (*imageapi.ImageStreamImport, error) {
+	from := o.From
+	// update ImageStream appropriately
+	if len(from) == 0 {
+		if len(stream.Spec.DockerImageRepository) != 0 {
+			from = stream.Spec.DockerImageRepository
+		} else {
+			tags := make(map[string]string)
+			for name, tag := range stream.Spec.Tags {
+				if tag.From != nil && tag.From.Kind == "DockerImage" {
+					tags[name] = tag.From.Name
 				}
-				from = stream.Spec.DockerImageRepository
 			}
-			if from != stream.Spec.DockerImageRepository {
-				if !o.Confirm {
-					if len(stream.Spec.DockerImageRepository) == 0 {
-						return nil, nil, fmt.Errorf("the image stream does not currently import an entire Docker repository, pass --confirm to update")
-					}
-					return nil, nil, fmt.Errorf("the image stream has a different import spec %q, pass --confirm to update", stream.Spec.DockerImageRepository)
-				}
-				stream.Spec.DockerImageRepository = from
+			if len(tags) == 0 {
+				return nil, fmt.Errorf("image stream does not have tags pointing to external docker images")
 			}
+			return o.newImageStreamImportTags(stream, tags), nil
+		}
+	}
+	if from != stream.Spec.DockerImageRepository {
+		if !o.Confirm {
+			if len(stream.Spec.DockerImageRepository) == 0 {
+				return nil, fmt.Errorf("the image stream does not currently import an entire Docker repository, pass --confirm to update")
+			}
+			return nil, fmt.Errorf("the image stream has a different import spec %q, pass --confirm to update", stream.Spec.DockerImageRepository)
+		}
+		stream.Spec.DockerImageRepository = from
+	}
 
-		} else {
-			// importing a single tag
+	// and create accompanying ImageStreamImport
+	return o.newImageStreamImportAll(stream, from), nil
+}
 
-			// follow any referential tags to the destination
-			finalTag, existing, ok, multiple := imageapi.FollowTagReference(stream, tag)
-			if !ok && multiple {
-				return nil, nil, fmt.Errorf("tag %q on the image stream is a reference to %q, which does not exist", tag, finalTag)
-			}
+func (o *ImportImageOptions) importTag(stream *imageapi.ImageStream) (*imageapi.ImageStreamImport, error) {
+	from := o.From
+	tag := o.Tag
+	// follow any referential tags to the destination
+	finalTag, existing, ok, multiple := imageapi.FollowTagReference(stream, tag)
+	if !ok && multiple {
+		return nil, fmt.Errorf("tag %q on the image stream is a reference to %q, which does not exist", tag, finalTag)
+	}
 
-			if ok {
-				// disallow changing an existing tag
-				if existing.From == nil || existing.From.Kind != "DockerImage" {
-					return nil, nil, fmt.Errorf("tag %q already exists - you must use the 'tag' command if you want to change the source to %q", tag, from)
-				}
-				if len(from) != 0 && from != existing.From.Name {
-					if multiple {
-						return nil, nil, fmt.Errorf("the tag %q points to the tag %q which points to %q - use the 'tag' command if you want to change the source to %q", tag, finalTag, existing.From.Name, from)
-					}
-					return nil, nil, fmt.Errorf("the tag %q points to %q - use the 'tag' command if you want to change the source to %q", tag, existing.From.Name, from)
-				}
+	// update ImageStream appropriately
+	if ok {
+		// disallow changing an existing tag
+		if existing.From == nil || existing.From.Kind != "DockerImage" {
+			return nil, fmt.Errorf("tag %q already exists - you must use the 'tag' command if you want to change the source to %q", tag, from)
+		}
+		if len(from) != 0 && from != existing.From.Name {
+			if multiple {
+				return nil, fmt.Errorf("the tag %q points to the tag %q which points to %q - use the 'tag' command if you want to change the source to %q", tag, finalTag, existing.From.Name, from)
+			}
+			return nil, fmt.Errorf("the tag %q points to %q - use the 'tag' command if you want to change the source to %q", tag, existing.From.Name, from)
+		}
 
-				// set the target item to import
-				from = existing.From.Name
-				if multiple {
-					tag = finalTag
-				}
+		// set the target item to import
+		from = existing.From.Name
+		if multiple {
+			tag = finalTag
+		}
 
-				// clear the legacy annotation
-				delete(existing.Annotations, imageapi.DockerImageRepositoryCheckAnnotation)
-				// reset the generation
-				zero := int64(0)
-				existing.Generation = &zero
+		// clear the legacy annotation
+		delete(existing.Annotations, imageapi.DockerImageRepositoryCheckAnnotation)
+		// reset the generation
+		zero := int64(0)
+		existing.Generation = &zero
 
-			} else {
-				// create a new tag
-				if len(from) == 0 {
-					from = stream.Spec.DockerImageRepository
-				}
-				// if the from is still empty this means there's no such tag defined
-				// nor we can't create any from .spec.dockerImageRepository
-				if len(from) == 0 {
-					return nil, nil, fmt.Errorf("the tag %q does not exist on the image stream - choose an existing tag to import or use the 'tag' command to create a new tag", tag)
-				}
-				existing = &imageapi.TagReference{
-					From: &kapi.ObjectReference{
-						Kind: "DockerImage",
-						Name: from,
-					},
-				}
-			}
-			stream.Spec.Tags[tag] = *existing
+	} else {
+		// create a new tag
+		if len(from) == 0 {
+			from = stream.Spec.DockerImageRepository
+		}
+		// if the from is still empty this means there's no such tag defined
+		// nor we can't create any from .spec.dockerImageRepository
+		if len(from) == 0 {
+			return nil, fmt.Errorf("the tag %q does not exist on the image stream - choose an existing tag to import or use the 'tag' command to create a new tag", tag)
+		}
+		existing = &imageapi.TagReference{
+			From: &kapi.ObjectReference{
+				Kind: "DockerImage",
+				Name: from,
+			},
 		}
 	}
+	stream.Spec.Tags[tag] = *existing
 
+	// and create accompanying ImageStreamImport
+	return o.newImageStreamImportTags(stream, map[string]string{tag: from}), nil
+}
+
+func (o *ImportImageOptions) newImageStream() (*imageapi.ImageStream, *imageapi.ImageStreamImport) {
+	from := o.From
+	tag := o.Tag
 	if len(from) == 0 {
-		// catch programmer error
-		return nil, nil, fmt.Errorf("unexpected error, from is empty")
+		from = o.Target
+	}
+	var stream *imageapi.ImageStream
+	// create new ImageStream
+	if o.All {
+		stream = &imageapi.ImageStream{
+			ObjectMeta: kapi.ObjectMeta{Name: o.Name},
+			Spec:       imageapi.ImageStreamSpec{DockerImageRepository: from},
+		}
+	} else {
+		stream = &imageapi.ImageStream{
+			ObjectMeta: kapi.ObjectMeta{Name: o.Name},
+			Spec: imageapi.ImageStreamSpec{
+				Tags: map[string]imageapi.TagReference{
+					tag: {
+						From: &kapi.ObjectReference{
+							Kind: "DockerImage",
+							Name: from,
+						},
+					},
+				},
+			},
+		}
+	}
+	// and accompanying ImageStreamImport
+	var isi *imageapi.ImageStreamImport
+	if o.All {
+		isi = o.newImageStreamImportAll(stream, from)
+	} else {
+		isi = o.newImageStreamImportTags(stream, map[string]string{tag: from})
 	}
 
-	// Attempt the new, direct import path
+	return stream, isi
+}
+
+func (o *ImportImageOptions) newImageStreamImport(stream *imageapi.ImageStream) (*imageapi.ImageStreamImport, bool) {
 	isi := &imageapi.ImageStreamImport{
 		ObjectMeta: kapi.ObjectMeta{
 			Name:            stream.Name,
@@ -384,15 +429,26 @@ func (o *ImportImageOptions) createImageImport() (*imageapi.ImageStream, *imagea
 	if o.Insecure != nil {
 		insecure = *o.Insecure
 	}
-	if o.All {
-		isi.Spec.Repository = &imageapi.RepositoryImportSpec{
-			From: kapi.ObjectReference{
-				Kind: "DockerImage",
-				Name: from,
-			},
-			ImportPolicy: imageapi.TagImportPolicy{Insecure: insecure},
-		}
-	} else {
+
+	return isi, insecure
+}
+
+func (o *ImportImageOptions) newImageStreamImportAll(stream *imageapi.ImageStream, from string) *imageapi.ImageStreamImport {
+	isi, insecure := o.newImageStreamImport(stream)
+	isi.Spec.Repository = &imageapi.RepositoryImportSpec{
+		From: kapi.ObjectReference{
+			Kind: "DockerImage",
+			Name: from,
+		},
+		ImportPolicy: imageapi.TagImportPolicy{Insecure: insecure},
+	}
+
+	return isi
+}
+
+func (o *ImportImageOptions) newImageStreamImportTags(stream *imageapi.ImageStream, tags map[string]string) *imageapi.ImageStreamImport {
+	isi, insecure := o.newImageStreamImport(stream)
+	for tag, from := range tags {
 		isi.Spec.Images = append(isi.Spec.Images, imageapi.ImageImportSpec{
 			From: kapi.ObjectReference{
 				Kind: "DockerImage",
@@ -402,6 +458,5 @@ func (o *ImportImageOptions) createImageImport() (*imageapi.ImageStream, *imagea
 			ImportPolicy: imageapi.TagImportPolicy{Insecure: insecure},
 		})
 	}
-
-	return stream, isi, nil
+	return isi
 }