Merge pull request #7936 from rhcarvalho/issue7718-build-output-cycle…

…-detection

Merged by openshift-bot

pkg/build/api/v1/conversion.go

@@ -19,7 +19,7 @@ func convert_v1_BuildConfig_To_api_BuildConfig(in *BuildConfig, out *newer.Build
 	// strip off any default imagechange triggers where the buildconfig's
 	// "from" is not an ImageStreamTag, because those triggers
 	// will never be invoked.
-	imageRef := buildutil.GetImageStreamForStrategy(out.Spec.Strategy)
+	imageRef := buildutil.GetInputReference(out.Spec.Strategy)
 	hasIST := imageRef != nil && imageRef.Kind == "ImageStreamTag"
 	for _, trigger := range out.Spec.Triggers {
 		if trigger.Type != newer.ImageChangeBuildTriggerType {

pkg/build/api/v1beta3/conversion.go

@@ -20,7 +20,7 @@ func convert_v1beta3_BuildConfig_To_api_BuildConfig(in *BuildConfig, out *newer.
 	// strip off any default imagechange triggers where the buildconfig's
 	// "from" is not an ImageStreamTag, because those triggers
 	// will never be invoked.
-	imageRef := buildutil.GetImageStreamForStrategy(out.Spec.Strategy)
+	imageRef := buildutil.GetInputReference(out.Spec.Strategy)
 	hasIST := imageRef != nil && imageRef.Kind == "ImageStreamTag"
 	for _, trigger := range out.Spec.Triggers {
 		if trigger.Type != newer.ImageChangeBuildTriggerType {

pkg/build/api/validation/validation.go

@@ -64,7 +64,7 @@ func ValidateBuildConfig(config *buildapi.BuildConfig) field.ErrorList {
 	fromRefs := map[string]struct{}{}
 	specPath := field.NewPath("spec")
 	triggersPath := specPath.Child("triggers")
-	buildFrom := buildutil.GetImageStreamForStrategy(config.Spec.Strategy)
+	buildFrom := buildutil.GetInputReference(config.Spec.Strategy)
 	for i, trg := range config.Spec.Triggers {
 		allErrs = append(allErrs, validateTrigger(&trg, buildFrom, triggersPath.Index(i))...)
 		if trg.Type != buildapi.ImageChangeBuildTriggerType || trg.ImageChange == nil {

pkg/build/controller/image_change_controller.go

@@ -72,7 +72,7 @@ func (c *ImageChangeController) HandleImageRepo(repo *imageapi.ImageStream) erro
 			if trigger.ImageChange.From != nil {
 				from = trigger.ImageChange.From
 			} else {
-				from = buildutil.GetImageStreamForStrategy(config.Spec.Strategy)
+				from = buildutil.GetInputReference(config.Spec.Strategy)
 			}
 
 			if from == nil || from.Kind != "ImageStreamTag" {

pkg/build/generator/generator.go

@@ -143,7 +143,7 @@ func findImageChangeTrigger(bc *buildapi.BuildConfig, ref *kapi.ObjectReference)
 		imageChange := trigger.ImageChange
 		triggerRef := imageChange.From
 		if triggerRef == nil {
-			triggerRef = buildutil.GetImageStreamForStrategy(bc.Spec.Strategy)
+			triggerRef = buildutil.GetInputReference(bc.Spec.Strategy)
 			if triggerRef == nil || triggerRef.Kind != "ImageStreamTag" {
 				continue
 			}
@@ -288,7 +288,7 @@ func (g *BuildGenerator) updateImageTriggers(ctx kapi.Context, bc *buildapi.Buil
 
 		triggerImageRef := trigger.ImageChange.From
 		if triggerImageRef == nil {
-			triggerImageRef = buildutil.GetImageStreamForStrategy(bc.Spec.Strategy)
+			triggerImageRef = buildutil.GetInputReference(bc.Spec.Strategy)
 		}
 		if triggerImageRef == nil {
 			glog.Warningf("Could not get ImageStream reference for default ImageChangeTrigger on BuildConfig %s/%s", bc.Namespace, bc.Name)
@@ -434,7 +434,7 @@ func (g *BuildGenerator) generateBuildFromConfig(ctx kapi.Context, bc *buildapi.
 		var sourceImageSpec string
 		// if the imagesource matches the strategy from, and we have a trigger for the strategy from,
 		// use the imageid from the trigger rather than resolving it.
-		if strategyFrom := buildutil.GetImageStreamForStrategy(bc.Spec.Strategy); reflect.DeepEqual(sourceImage.From, *strategyFrom) &&
+		if strategyFrom := buildutil.GetInputReference(bc.Spec.Strategy); reflect.DeepEqual(sourceImage.From, *strategyFrom) &&
 			strategyImageChangeTrigger != nil {
 			sourceImageSpec = strategyImageChangeTrigger.LastTriggeredImageID
 		} else {

pkg/build/generator/generator_test.go

@@ -299,7 +299,7 @@ func TestInstantiateWithImageTrigger(t *testing.T) {
 			if bc.Spec.Triggers[i].Type == buildapi.ImageChangeBuildTriggerType {
 				from := bc.Spec.Triggers[i].ImageChange.From
 				if from == nil {
-					from = buildutil.GetImageStreamForStrategy(bc.Spec.Strategy)
+					from = buildutil.GetInputReference(bc.Spec.Strategy)
 				}
 				if bc.Spec.Triggers[i].ImageChange.LastTriggeredImageID != ("ref@" + from.Name) {
 					t.Errorf("%s: expected LastTriggeredImageID for trigger at %d to be %s. Got: %s", tc.name, i, "ref@"+from.Name, bc.Spec.Triggers[i].ImageChange.LastTriggeredImageID)

pkg/build/graph/edges.go

@@ -94,7 +94,7 @@ func AddInputEdges(g osgraph.MutableUniqueGraph, node *buildgraph.BuildConfigNod
 	if in := buildgraph.EnsureSourceRepositoryNode(g, node.BuildConfig.Spec.Source); in != nil {
 		g.AddEdge(in, node, BuildInputEdgeKind)
 	}
-	inputImage := buildutil.GetImageStreamForStrategy(node.BuildConfig.Spec.Strategy)
+	inputImage := buildutil.GetInputReference(node.BuildConfig.Spec.Strategy)
 	if input := imageRefNode(g, inputImage, node.BuildConfig); input != nil {
 		g.AddEdge(input, node, BuildInputImageEdgeKind)
 	}
@@ -108,7 +108,7 @@ func AddTriggerEdges(g osgraph.MutableUniqueGraph, node *buildgraph.BuildConfigN
 		}
 		from := trigger.ImageChange.From
 		if trigger.ImageChange.From == nil {
-			from = buildutil.GetImageStreamForStrategy(node.BuildConfig.Spec.Strategy)
+			from = buildutil.GetInputReference(node.BuildConfig.Spec.Strategy)
 		}
 		triggerNode := imageRefNode(g, from, node.BuildConfig)
 		g.AddEdge(triggerNode, node, BuildTriggerImageEdgeKind)

pkg/build/util/util.go

@@ -30,9 +30,9 @@ func GetBuildName(pod *kapi.Pod) string {
 	return pod.Annotations[buildapi.BuildAnnotation]
 }
 
-// GetImageStreamForStrategy returns the ImageStream[Tag/Image] ObjectReference associated
-// with the BuildStrategy.
-func GetImageStreamForStrategy(strategy buildapi.BuildStrategy) *kapi.ObjectReference {
+// GetInputReference returns the From ObjectReference associated with the
+// BuildStrategy.
+func GetInputReference(strategy buildapi.BuildStrategy) *kapi.ObjectReference {
 	switch {
 	case strategy.SourceStrategy != nil:
 		return &strategy.SourceStrategy.From

pkg/cmd/cli/cmd/set/triggers.go

@@ -672,7 +672,7 @@ Outer:
 // strategyTrigger returns a synthetic ImageChangeTrigger that represents the image stream tag the build strategy
 // points to, or nil if no such strategy trigger is possible (if the build doesn't point to an ImageStreamTag).
 func strategyTrigger(config *buildapi.BuildConfig) *ImageChangeTrigger {
-	if from := buildutil.GetImageStreamForStrategy(config.Spec.Strategy); from != nil {
+	if from := buildutil.GetInputReference(config.Spec.Strategy); from != nil {
 		if from.Kind == "ImageStreamTag" {
 			// normalize the strategy object reference
 			from.Namespace = defaultNamespace(from.Namespace, config.Namespace)

pkg/generate/app/cmd/newapp.go

@@ -1097,17 +1097,20 @@ func (c *AppConfig) run(acceptors app.Acceptors) (*AppResult, error) {
 		}
 	}
 
-	err = c.checkCircularReferences(objects)
-	if err != nil {
-		if err, ok := err.(app.CircularOutputReferenceError); ok {
-			if len(c.To) == 0 {
-				// Output reference was generated, return error.
-				return nil, fmt.Errorf("%v, set a different tag with --to", err)
+	// Only check circular references for `oc new-build`.
+	if c.ExpectToBuild {
+		err = c.checkCircularReferences(objects)
+		if err != nil {
+			if err, ok := err.(app.CircularOutputReferenceError); ok {
+				if len(c.To) == 0 {
+					// Output reference was generated, return error.
+					return nil, fmt.Errorf("%v, set a different tag with --to", err)
+				}
+				// Output reference was explicitly provided, print warning.
+				fmt.Fprintf(c.ErrOut, "--> WARNING: %v\n", err)
+			} else {
+				return nil, err
 			}
-			// Output reference was explicitly provided, print warning.
-			fmt.Fprintf(c.ErrOut, "--> WARNING: %v\n", err)
-		} else {
-			return nil, err
 		}
 	}
 
@@ -1125,7 +1128,7 @@ func (c *AppConfig) run(acceptors app.Acceptors) (*AppResult, error) {
 func (c *AppConfig) checkCircularReferences(objects app.Objects) error {
 	for _, obj := range objects {
 		if bc, ok := obj.(*buildapi.BuildConfig); ok {
-			input := buildutil.GetImageStreamForStrategy(bc.Spec.Strategy)
+			input := buildutil.GetInputReference(bc.Spec.Strategy)
 			if bc.Spec.Output.To != nil && input != nil &&
 				reflect.DeepEqual(input, bc.Spec.Output.To) {
 				ns := input.Namespace

pkg/image/prune/imagepruner.go

@@ -495,7 +495,7 @@ func addBuildsToGraph(g graph.Graph, builds *buildapi.BuildList) {
 // to the image specified by strategy.from, as long as the image is managed by
 // OpenShift.
 func addBuildStrategyImageReferencesToGraph(g graph.Graph, strategy buildapi.BuildStrategy, predecessor gonum.Node) {
-	from := buildutil.GetImageStreamForStrategy(strategy)
+	from := buildutil.GetInputReference(strategy)
 	if from == nil {
 		glog.V(4).Infof("Unable to determine 'from' reference - skipping")
 		return

test/extended/builds/dockerfile.go

@@ -34,7 +34,7 @@ USER 1001
 	g.Describe("being created from new-build", func() {
 		g.It("should create a image via new-build", func() {
 			g.By(fmt.Sprintf("calling oc new-build with Dockerfile"))
-			err := oc.Run("new-build").Args("-D", "-").InputString(testDockerfile).Execute()
+			err := oc.Run("new-build").Args("-D", "-", "--to", "origin-base:custom").InputString(testDockerfile).Execute()
 			o.Expect(err).NotTo(o.HaveOccurred())
 
 			g.By("starting a test build")

test/integration/newapp_test.go

@@ -918,50 +918,6 @@ func TestNewAppRunBuilds(t *testing.T) {
 				return fmt.Errorf("BuildConfig not found; got %v", res.List.Items)
 			},
 		},
-		{
-			name: "successful build from dockerfile with identical input and output image references with warning",
-			config: &cmd.AppConfig{
-				Dockerfile: "FROM centos\nRUN yum install -y httpd",
-				To:         "centos",
-			},
-			expected: map[string][]string{
-				"buildConfig": {"centos"},
-				"imageStream": {"centos"},
-			},
-			checkOutput: func(stdout, stderr io.Reader) error {
-				got, err := ioutil.ReadAll(stderr)
-				if err != nil {
-					return err
-				}
-				want := "--> WARNING: output image of \"default/centos:latest\" must be different than input\n"
-				if string(got) != want {
-					return fmt.Errorf("stderr: got %q; want %q", got, want)
-				}
-				return nil
-			},
-		},
-		{
-			name: "successful build from dockerfile with identical input and output image references with warning",
-			config: &cmd.AppConfig{
-				Dockerfile: "FROM openshift/ruby-22-centos7\nRUN yum install -y httpd",
-				To:         "ruby-22-centos7",
-			},
-			expected: map[string][]string{
-				"buildConfig": {"ruby-22-centos7"},
-				"imageStream": {"ruby-22-centos7"},
-			},
-			checkOutput: func(stdout, stderr io.Reader) error {
-				got, err := ioutil.ReadAll(stderr)
-				if err != nil {
-					return err
-				}
-				want := "--> WARNING: output image of \"default/ruby-22-centos7:latest\" must be different than input\n"
-				if string(got) != want {
-					return fmt.Errorf("stderr: got %q; want %q", got, want)
-				}
-				return nil
-			},
-		},
 		{
 			name: "successful generation of BC with multiple sources: repo + Dockerfile",
 			config: &cmd.AppConfig{
@@ -1017,30 +973,6 @@ func TestNewAppRunBuilds(t *testing.T) {
 				return err.Error() == "the Dockerfile in the repository \"\" has no FROM instruction"
 			},
 		},
-		{
-			name: "unsuccessful build from dockerfile due to identical input and output image references",
-			config: &cmd.AppConfig{
-				Dockerfile: "FROM centos\nRUN yum install -y httpd",
-			},
-			expectedErr: func(err error) bool {
-				e := app.CircularOutputReferenceError{
-					Reference: "default/centos:latest",
-				}
-				return err.Error() == fmt.Errorf("%v, set a different tag with --to", e).Error()
-			},
-		},
-		{
-			name: "unsuccessful build from dockerfile due to identical input and output image references",
-			config: &cmd.AppConfig{
-				Dockerfile: "FROM openshift/ruby-22-centos7\nRUN yum install -y httpd",
-			},
-			expectedErr: func(err error) bool {
-				e := app.CircularOutputReferenceError{
-					Reference: "default/ruby-22-centos7:latest",
-				}
-				return err.Error() == fmt.Errorf("%v, set a different tag with --to", e).Error()
-			},
-		},
 		{
 			name: "unsuccessful generation of BC with multiple repos and Dockerfile",
 			config: &cmd.AppConfig{
@@ -1220,6 +1152,134 @@ func TestNewAppRunBuilds(t *testing.T) {
 	}
 }
 
+func TestBuildOutputCycleDetection(t *testing.T) {
+	skipExternalGit(t)
+	tests := []struct {
+		name   string
+		config *cmd.AppConfig
+
+		expected    map[string][]string
+		expectedErr func(error) bool
+		checkOutput func(stdout, stderr io.Reader) error
+	}{
+		{
+			name: "successful build from dockerfile with identical input and output image references with warning",
+			config: &cmd.AppConfig{
+				Dockerfile: "FROM centos\nRUN yum install -y httpd",
+				To:         "centos",
+			},
+			expected: map[string][]string{
+				"buildConfig": {"centos"},
+				"imageStream": {"centos"},
+			},
+			checkOutput: func(stdout, stderr io.Reader) error {
+				got, err := ioutil.ReadAll(stderr)
+				if err != nil {
+					return err
+				}
+				want := "--> WARNING: output image of \"default/centos:latest\" must be different than input\n"
+				if string(got) != want {
+					return fmt.Errorf("stderr: got %q; want %q", got, want)
+				}
+				return nil
+			},
+		},
+		{
+			name: "successful build from dockerfile with identical input and output image references with warning",
+			config: &cmd.AppConfig{
+				Dockerfile: "FROM openshift/ruby-22-centos7\nRUN yum install -y httpd",
+				To:         "ruby-22-centos7",
+			},
+			expected: map[string][]string{
+				"buildConfig": {"ruby-22-centos7"},
+				"imageStream": {"ruby-22-centos7"},
+			},
+			checkOutput: func(stdout, stderr io.Reader) error {
+				got, err := ioutil.ReadAll(stderr)
+				if err != nil {
+					return err
+				}
+				want := "--> WARNING: output image of \"default/ruby-22-centos7:latest\" must be different than input\n"
+				if string(got) != want {
+					return fmt.Errorf("stderr: got %q; want %q", got, want)
+				}
+				return nil
+			},
+		},
+		{
+			name: "unsuccessful build from dockerfile due to identical input and output image references",
+			config: &cmd.AppConfig{
+				Dockerfile: "FROM centos\nRUN yum install -y httpd",
+			},
+			expectedErr: func(err error) bool {
+				e := app.CircularOutputReferenceError{
+					Reference: "default/centos:latest",
+				}
+				return err.Error() == fmt.Errorf("%v, set a different tag with --to", e).Error()
+			},
+		},
+		{
+			name: "unsuccessful build from dockerfile due to identical input and output image references",
+			config: &cmd.AppConfig{
+				Dockerfile: "FROM openshift/ruby-22-centos7\nRUN yum install -y httpd",
+			},
+			expectedErr: func(err error) bool {
+				e := app.CircularOutputReferenceError{
+					Reference: "default/ruby-22-centos7:latest",
+				}
+				return err.Error() == fmt.Errorf("%v, set a different tag with --to", e).Error()
+			},
+		},
+	}
+	for _, test := range tests {
+		stdout, stderr := PrepareAppConfig(test.config)
+
+		res, err := test.config.Run()
+		if (test.expectedErr == nil && err != nil) || (test.expectedErr != nil && !test.expectedErr(err)) {
+			t.Errorf("%s: unexpected error: %v", test.name, err)
+			continue
+		}
+		if err != nil {
+			continue
+		}
+		if test.checkOutput != nil {
+			if err := test.checkOutput(stdout, stderr); err != nil {
+				t.Error(err)
+				continue
+			}
+		}
+		got := map[string][]string{}
+		for _, obj := range res.List.Items {
+			switch tp := obj.(type) {
+			case *buildapi.BuildConfig:
+				got["buildConfig"] = append(got["buildConfig"], tp.Name)
+			case *imageapi.ImageStream:
+				got["imageStream"] = append(got["imageStream"], tp.Name)
+			}
+		}
+
+		if len(test.expected) != len(got) {
+			t.Errorf("%s: Resource kind size mismatch! Expected %d, got %d", test.name, len(test.expected), len(got))
+			continue
+		}
+
+		for k, exp := range test.expected {
+			g, ok := got[k]
+			if !ok {
+				t.Errorf("%s: Didn't find expected kind %s", test.name, k)
+			}
+
+			sort.Strings(g)
+			sort.Strings(exp)
+
+			if !reflect.DeepEqual(g, exp) {
+				t.Errorf("%s: Resource names mismatch! Expected %v, got %v", test.name, exp, g)
+				continue
+			}
+		}
+	}
+}
+
 func TestNewAppNewBuildEnvVars(t *testing.T) {
 	skipExternalGit(t)
 	dockerSearcher := app.DockerRegistrySearcher{