Merge pull request #628 from php-coder/gh522_fix_callback_url

Merged by openshift-bot

docs/cli.md

@@ -71,7 +71,7 @@ that image and add them to the tar streamed to the container into `/artifacts`.
 
 | Name                       | Description                                             |
 |:-------------------------- |:--------------------------------------------------------|
-| `--callback-url`           | URL to be invoked after a successful build (see [Callback URL](#callback-url)) |
+| `--callback-url`           | URL to be invoked after a build (see [Callback URL](#callback-url)) |
 | `-c (--copy)`              | Use local file system copy instead of git cloning the source url (allows for inclusion of empty directories and uncommitted files) |
 | `-d (--destination)`       | Location where the scripts and sources will be placed prior doing build (see [S2I Scripts](https://github.com/openshift/source-to-image/blob/master/docs/builder_image.md#s2i-scripts)) |
 | `--dockercfg-path`         | The path to the Docker configuration file |
@@ -129,12 +129,18 @@ about the build:
 
 * `success` - flag indicating the result of the build process (`true` or `false`)
 * `payload` - list of messages from the build process
+* `labels`  - labels of the resulting image
 
 Example: data posted will be in the form:
 ```
 {
     "payload": "A string containing all build messages",
-    "success": true
+    "success": true,
+    "labels": {
+        "io.k8s.display-name": "my-app",
+        "io.openshift.s2i.build.image": "builder-image:latest",
+        ...
+    }
 }
 ```
 

pkg/build/strategies/sti/postexecutorstep.go

@@ -39,7 +39,7 @@ type postExecutorStepContext struct {
 
 	// Labels that will be passed to a callback.
 	// These labels are added to the image during commit.
-	// See also: commitImageStep and invokeCallbackStep
+	// See also: commitImageStep and STI.Build()
 	labels map[string]string
 }
 
@@ -377,23 +377,6 @@ func (step *reportSuccessStep) execute(ctx *postExecutorStepContext) error {
 	return nil
 }
 
-type invokeCallbackStep struct {
-	builder         *STI
-	callbackInvoker util.CallbackInvoker
-}
-
-func (step *invokeCallbackStep) execute(ctx *postExecutorStepContext) error {
-	if len(step.builder.config.CallbackURL) > 0 {
-		glog.V(3).Info("Executing step: invoke callback url")
-		step.builder.result.Messages = step.callbackInvoker.ExecuteCallback(step.builder.config.CallbackURL,
-			step.builder.result.Success, ctx.labels, step.builder.result.Messages)
-		return nil
-	}
-
-	glog.V(3).Info("Skipping step: invoke callback url")
-	return nil
-}
-
 // shared methods
 
 func commitContainer(docker dockerpkg.Docker, containerID, cmd, user, tag string, env, entrypoint []string, labels map[string]string) (string, error) {

pkg/build/strategies/sti/postexecutorstep_test.go

@@ -6,7 +6,6 @@ import (
 	"testing"
 
 	"github.com/openshift/source-to-image/pkg/docker"
-	"github.com/openshift/source-to-image/pkg/test"
 )
 
 func TestStorePreviousImageStep(t *testing.T) {
@@ -235,49 +234,3 @@ func TestReportSuccessStep(t *testing.T) {
 		t.Errorf("should set ImageID field to %q but it's %q", ctx.imageID, builder.result.ImageID)
 	}
 }
-
-func TestInvokeCallbackStep(t *testing.T) {
-	expectedMessages := []string{"i'm", "ok"}
-	expectedCallbackURL := "http://ping.me"
-	builder := newFakeBaseSTI()
-	builder.result.Success = true
-	builder.result.Messages = expectedMessages
-	builder.config.CallbackURL = expectedCallbackURL
-
-	expectedResultMessages := []string{"all", "right"}
-	callbackInvoker := &test.FakeCallbackInvoker{}
-	callbackInvoker.Result = expectedResultMessages
-
-	step := &invokeCallbackStep{
-		builder:         builder,
-		callbackInvoker: callbackInvoker,
-	}
-
-	expectedLabels := make(map[string]string)
-	expectedLabels["result"] = "passed"
-	ctx := &postExecutorStepContext{labels: expectedLabels}
-
-	if err := step.execute(ctx); err != nil {
-		t.Fatalf("should exit without error, but it returned %v", err)
-	}
-
-	if !reflect.DeepEqual(builder.result.Messages, expectedResultMessages) {
-		t.Errorf("should set Messages field to %q but it's %q", expectedResultMessages, builder.result.Messages)
-	}
-
-	if callbackInvoker.CallbackURL != expectedCallbackURL {
-		t.Errorf("should invoke ExecuteCallback(CallbackURL=%q) but invoked with %q", expectedCallbackURL, callbackInvoker.CallbackURL)
-	}
-
-	if callbackInvoker.Success != true {
-		t.Errorf("should invoke ExecuteCallback(Success='true') but invoked with %v", callbackInvoker.Success)
-	}
-
-	if !reflect.DeepEqual(callbackInvoker.Messages, expectedMessages) {
-		t.Errorf("should invoke ExecuteCallback(Messages=%v) but invoked with %v", expectedMessages, callbackInvoker.Messages)
-	}
-
-	if !reflect.DeepEqual(callbackInvoker.Labels, expectedLabels) {
-		t.Errorf("should invoke ExecuteCallback(Labels=%v) but invoked with %v", expectedLabels, callbackInvoker.Labels)
-	}
-}

pkg/build/strategies/sti/sti.go

@@ -173,6 +173,12 @@ func New(config *api.Config, overrides build.Overrides) (*STI, error) {
 func (builder *STI) Build(config *api.Config) (*api.Result, error) {
 	builder.result = &api.Result{}
 
+	if len(builder.config.CallbackURL) > 0 {
+		defer func() {
+			builder.result.Messages = builder.callbackInvoker.ExecuteCallback(builder.config.CallbackURL,
+				builder.result.Success, builder.postExecutorStepsContext.labels, builder.result.Messages)
+		}()
+	}
 	defer builder.garbage.Cleanup(config)
 
 	glog.V(1).Infof("Preparing to build %s", config.Tag)
@@ -662,10 +668,6 @@ func (builder *STI) initPostExecutorSteps() {
 				builder: builder,
 				docker:  builder.docker,
 			},
-			&invokeCallbackStep{
-				builder:         builder,
-				callbackInvoker: builder.callbackInvoker,
-			},
 		}
 	} else {
 		builder.postExecutorFirstStageSteps = []postExecutorStep{
@@ -690,10 +692,6 @@ func (builder *STI) initPostExecutorSteps() {
 			&reportSuccessStep{
 				builder: builder,
 			},
-			&invokeCallbackStep{
-				builder:         builder,
-				callbackInvoker: builder.callbackInvoker,
-			},
 		}
 	}
 }

pkg/build/strategies/sti/sti_test.go

@@ -342,7 +342,6 @@ func TestPostExecute(t *testing.T) {
 		bh := testBuildHandler()
 		containerID := "test-container-id"
 		bh.result.Messages = []string{"one", "two"}
-		bh.config.CallbackURL = "https://my.callback.org/test"
 		bh.config.Tag = tc.tag
 		bh.config.Incremental = tc.incremental
 		dh := bh.docker.(*docker.FakeDocker)
@@ -351,7 +350,6 @@ func TestPostExecute(t *testing.T) {
 			bh.incremental = tc.incremental
 			bh.docker.(*docker.FakeDocker).GetImageIDResult = tc.previousImageID
 		}
-		ci := bh.callbackInvoker.(*test.FakeCallbackInvoker)
 		if tc.scriptsFromImage {
 			bh.scriptsURL = map[string]string{api.Run: "image:///usr/libexec/s2i/run"}
 		}
@@ -380,10 +378,6 @@ func TestPostExecute(t *testing.T) {
 				t.Errorf("(%d) Unexpected image removed: %s", i, dh.RemoveImageName)
 			}
 		}
-		// Ensure Callback was called
-		if ci.CallbackURL != bh.config.CallbackURL {
-			t.Errorf("(%d) Unexpected callbackURL, expected %q, got %q", i, bh.config.CallbackURL, ci.CallbackURL)
-		}
 	}
 }
 

pkg/util/callback.go

@@ -34,16 +34,19 @@ func (c *callbackInvoker) ExecuteCallback(callbackURL string, success bool, labe
 	}
 	writer.Flush()
 
-	d := map[string]interface{}{
-		"labels":  labels,
+	data := map[string]interface{}{
 		"payload": buf.String(),
 		"success": success,
 	}
 
+	if len(labels) > 0 {
+		data["labels"] = labels
+	}
+
 	jsonBuffer := new(bytes.Buffer)
 	writer = bufio.NewWriter(jsonBuffer)
 	jsonWriter := json.NewEncoder(writer)
-	jsonWriter.Encode(d)
+	jsonWriter.Encode(data)
 	writer.Flush()
 
 	var (

test/integration/integration_test.go

@@ -282,10 +282,11 @@ func (i *integrationTest) exerciseCleanBuild(tag string, verifyCallback bool, im
 				type CallbackMessage struct {
 					Payload string
 					Success bool
+					Labels  map[string]string
 				}
 				var callbackMessage CallbackMessage
 				err := json.Unmarshal(body, &callbackMessage)
-				callbackHasValidJSON = (err == nil) && (callbackMessage.Success)
+				callbackHasValidJSON = (err == nil) && callbackMessage.Success && len(callbackMessage.Labels) > 0
 			}
 		}
 		ts := httptest.NewServer(http.HandlerFunc(handler))