Merge pull request #394 from batikanu/fix-redirection-after-upload

Fix redirection after partly failing upload

Author: bryk

src/app/backend/apihandler.go

@@ -93,7 +93,7 @@ func CreateHttpApiHandler(client *client.Client, heapsterClient HeapsterClient)
 		deployFromFileWs.POST("").
 			To(apiHandler.handleDeployFromFile).
 			Reads(AppDeploymentFromFileSpec{}).
-			Writes(AppDeploymentFromFileSpec{}))
+			Writes(AppDeploymentFromFileResponse{}))
 	wsContainer.Add(deployFromFileWs)
 
 	replicationControllerWs := new(restful.WebService)
@@ -205,12 +205,23 @@ func (apiHandler *ApiHandler) handleDeployFromFile(request *restful.Request, res
 		handleInternalError(response, err)
 		return
 	}
-	if err := DeployAppFromFile(deploymentSpec, CreateObjectFromInfoFn); err != nil {
+
+	isDeployed, err := DeployAppFromFile(deploymentSpec, CreateObjectFromInfoFn)
+	if !isDeployed {
 		handleInternalError(response, err)
 		return
 	}
 
-	response.WriteHeaderAndEntity(http.StatusCreated, deploymentSpec)
+	errorMessage := ""
+	if err != nil {
+		errorMessage = err.Error()
+	}
+
+	response.WriteHeaderAndEntity(http.StatusCreated, AppDeploymentFromFileResponse{
+		Name: deploymentSpec.Name,
+		Content: deploymentSpec.Content,
+		Error: errorMessage,
+	})
 }
 
 // Handles app name validation API call.

src/app/backend/deploy.go

@@ -90,6 +90,18 @@ type AppDeploymentFromFileSpec struct {
 	Content string `json:"content"`
 }
 
+// Specification for deployment from file
+type AppDeploymentFromFileResponse struct {
+	// Name of the file
+	Name string `json:"name"`
+
+	// File content
+	Content string `json:"content"`
+
+	// Error after create resource
+	Error string `json:"error"`
+}
+
 // Port mapping for an application deployment.
 type PortMapping struct {
 	// Port that will be exposed on the service.
@@ -264,16 +276,16 @@ func getLabelsMap(labels []Label) map[string]string {
 	return result
 }
 
-type createObjectFromInfo func(info *kubectlResource.Info) error
+type createObjectFromInfo func(info *kubectlResource.Info) (bool, error)
 
 // Implementation of createObjectFromInfo
-func CreateObjectFromInfoFn(info *kubectlResource.Info) error {
-	_, err := kubectlResource.NewHelper(info.Client, info.Mapping).Create(info.Namespace, true, info.Object)
-	return err
+func CreateObjectFromInfoFn(info *kubectlResource.Info) (bool, error) {
+	createdResource, err := kubectlResource.NewHelper(info.Client, info.Mapping).Create(info.Namespace, true, info.Object)
+	return createdResource != nil , err
 }
 
 // Deploys an app based on the given yaml or json file.
-func DeployAppFromFile(spec *AppDeploymentFromFileSpec, createObjectFromInfoFn createObjectFromInfo) error {
+func DeployAppFromFile(spec *AppDeploymentFromFileSpec, createObjectFromInfoFn createObjectFromInfo) (bool, error) {
 	const (
 		validate      = true
 		emptyCacheDir = ""
@@ -282,7 +294,7 @@ func DeployAppFromFile(spec *AppDeploymentFromFileSpec, createObjectFromInfoFn c
 	factory := cmdutil.NewFactory(nil)
 	schema, err := factory.Validator(validate, emptyCacheDir)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	mapper, typer := factory.Object()
@@ -295,12 +307,16 @@ func DeployAppFromFile(spec *AppDeploymentFromFileSpec, createObjectFromInfoFn c
 		Flatten().
 		Do()
 
-	return r.Visit(func(info *kubectlResource.Info, err error) error {
-		err = createObjectFromInfoFn(info)
-		if err != nil {
-			return err
+	deployedResourcesCount:= 0
+
+	err = r.Visit(func(info *kubectlResource.Info, err error) error {
+		isDeployed, err := createObjectFromInfoFn(info)
+		if isDeployed {
+			deployedResourcesCount ++
+			log.Printf("%s is deployed", info.Name)
 		}
-		log.Printf("%s is deployed", info.Name)
-		return nil
+		return err
 	})
+
+	return deployedResourcesCount > 0, err
 }

src/app/frontend/deploy/deployfromfile_controller.js

@@ -84,9 +84,12 @@ export default class DeployFromFileController {
     let resource = this.resource_('api/v1/appdeploymentfromfile');
     resource.save(
         deploymentSpec,
-        (savedConfig) => {
-          defer.resolve(savedConfig);  // Progress ends
-          this.log_.info('Successfully deployed application: ', savedConfig);
+        (response) => {
+          defer.resolve(response);  // Progress ends
+          this.log_.info('Deployment is completed: ', response);
+          if (response.error.length > 0) {
+            this.errorDialog_.open('Deployment has been partly completed', response.error);
+          }
           this.state_.go(replicationcontrollerliststate);
         },
         (err) => {

src/test/backend/deploy_test.go

@@ -182,23 +182,23 @@ func TestGetAvailableProtocols(t *testing.T) {
 }
 
 func TestDeployAppFromFileWithValidContent(t *testing.T) {
-	const (
-		testNamespace = "test-deployfile-namespace"
-	)
 	validContent := "{\"kind\": \"Namespace\"," +
 		"\"apiVersion\": \"v1\"," +
 		"\"metadata\": {" +
-		"\"name\": \"" + testNamespace + "\"," +
+		"\"name\": \"test-deployfile-namespace\"," +
 		"\"labels\": {\"name\": \"development\"}}}"
 	spec := &AppDeploymentFromFileSpec{
 		Name:    "foo-name",
 		Content: validContent,
 	}
-	fakeCreateObjectFromInfo := func(info *kubectlResource.Info) error { return nil }
+	fakeCreateObjectFromInfo := func(info *kubectlResource.Info) (bool, error) { return true, nil }
 
-	err := DeployAppFromFile(spec, fakeCreateObjectFromInfo)
+	isDeployed, err := DeployAppFromFile(spec, fakeCreateObjectFromInfo)
 	if err != nil {
-		t.Errorf("Expected return value to be %#v but got %#v", nil, err)
+		t.Errorf("Expected return value to have %#v but got %#v", nil, err)
+	}
+	if !isDeployed {
+		t.Errorf("Expected return value to have %#v but got %#v", true, isDeployed)
 	}
 }
 
@@ -207,10 +207,14 @@ func TestDeployAppFromFileWithInvalidContent(t *testing.T) {
 		Name:    "foo-name",
 		Content: "foo-content-invalid",
 	}
-	fakeCreateObjectFromInfo := func(info *kubectlResource.Info) error { return nil }
+	// return is set to true to check if the validation prior to this function really works
+	fakeCreateObjectFromInfo := func(info *kubectlResource.Info) (bool, error) { return true, nil }
 
-	err := DeployAppFromFile(spec, fakeCreateObjectFromInfo)
+	isDeployed, err := DeployAppFromFile(spec, fakeCreateObjectFromInfo)
 	if err == nil {
-		t.Errorf("Expected return value to be an error but got %#v", nil)
+		t.Errorf("Expected return value to have an error but got %#v", nil)
+	}
+	if isDeployed {
+		t.Errorf("Expected return value to have %#v but got %#v", false, isDeployed)
 	}
 }

src/test/frontend/deploy/deployfromfile_controller_test.js

@@ -22,7 +22,6 @@ describe('DeployFromFile controller', () => {
   let mockResource;
   /** @type {!angular.FormController} */
   let form;
-
   beforeEach(() => {
     angular.mock.module(deployModule.name);
 
@@ -52,4 +51,62 @@ describe('DeployFromFile controller', () => {
     expect(resourceObject.save).toHaveBeenCalled();
   });
 
+  describe('After deploy', () => {
+    let httpBackend;
+    beforeEach(() => {
+      angular.mock.inject(($controller, $resource, $httpBackend) => {
+        ctrl = $controller(DeployFromFileController, {$resource: $resource}, {form: form});
+        httpBackend = $httpBackend;
+      });
+    });
+
+    it('should open error dialog and redirect the page', () => {
+      spyOn(ctrl.errorDialog_, 'open');
+      spyOn(ctrl.state_, 'go');
+      let response = {
+        name: 'foo-name',
+        content: 'foo-content',
+        error: 'service already exists',
+      };
+      httpBackend.expectPOST('api/v1/appdeploymentfromfile').respond(201, response);
+      // when
+      ctrl.deploy();
+      httpBackend.flush();
+
+      // then
+      expect(ctrl.errorDialog_.open).toHaveBeenCalled();
+      expect(ctrl.state_.go).toHaveBeenCalled();
+    });
+
+    it('should redirect the page and not open error dialog', () => {
+      spyOn(ctrl.errorDialog_, 'open');
+      spyOn(ctrl.state_, 'go');
+      let response = {
+        name: 'foo-name',
+        content: 'foo-content',
+        error: '',
+      };
+      httpBackend.expectPOST('api/v1/appdeploymentfromfile').respond(201, response);
+      // when
+      ctrl.deploy();
+      httpBackend.flush();
+
+      // then
+      expect(ctrl.errorDialog_.open).not.toHaveBeenCalled();
+      expect(ctrl.state_.go).toHaveBeenCalled();
+    });
+
+    it('should not redirect the page and but open error dialog', () => {
+      spyOn(ctrl.errorDialog_, 'open');
+      spyOn(ctrl.state_, 'go');
+      httpBackend.expectPOST('api/v1/appdeploymentfromfile').respond(500, "Deployment failed");
+      // when
+      ctrl.deploy();
+      httpBackend.flush();
+
+      // then
+      expect(ctrl.errorDialog_.open).toHaveBeenCalled();
+      expect(ctrl.state_.go).not.toHaveBeenCalled();
+    });
+  });
 });