From 5b2f0ee81fa6b33c4684bb79e4fb9467b9a4479b Mon Sep 17 00:00:00 2001 From: Slava Semushin Date: Tue, 1 Nov 2016 16:49:44 +0100 Subject: [PATCH] Invoke callback URL when build has failed. --- docs/cli.md | 10 +++- pkg/build/strategies/sti/postexecutorstep.go | 19 +------- .../strategies/sti/postexecutorstep_test.go | 47 ------------------- pkg/build/strategies/sti/sti.go | 14 +++--- pkg/build/strategies/sti/sti_test.go | 6 --- pkg/util/callback.go | 9 ++-- test/integration/integration_test.go | 3 +- 7 files changed, 23 insertions(+), 85 deletions(-) diff --git a/docs/cli.md b/docs/cli.md index 96090dbd2..5b9db936a 100644 --- a/docs/cli.md +++ b/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", + ... + } } ``` diff --git a/pkg/build/strategies/sti/postexecutorstep.go b/pkg/build/strategies/sti/postexecutorstep.go index 82b6df36f..d816b926a 100644 --- a/pkg/build/strategies/sti/postexecutorstep.go +++ b/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) { diff --git a/pkg/build/strategies/sti/postexecutorstep_test.go b/pkg/build/strategies/sti/postexecutorstep_test.go index 9ea02d798..7b249e75c 100644 --- a/pkg/build/strategies/sti/postexecutorstep_test.go +++ b/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) - } -} diff --git a/pkg/build/strategies/sti/sti.go b/pkg/build/strategies/sti/sti.go index 018e8c50c..721538a0f 100644 --- a/pkg/build/strategies/sti/sti.go +++ b/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, - }, } } } diff --git a/pkg/build/strategies/sti/sti_test.go b/pkg/build/strategies/sti/sti_test.go index f86582384..1daf054ac 100644 --- a/pkg/build/strategies/sti/sti_test.go +++ b/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) - } } } diff --git a/pkg/util/callback.go b/pkg/util/callback.go index 281c8e4d5..0721ec020 100644 --- a/pkg/util/callback.go +++ b/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 ( diff --git a/test/integration/integration_test.go b/test/integration/integration_test.go index 83567dbc6..092a9ae79 100644 --- a/test/integration/integration_test.go +++ b/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))