Refactor positioning of webhook calls

caraml-dev · Nov 5, 2024 · 9eece20 · 9eece20
1 parent fc903e5
commit 9eece20
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 78 deletions.
diff --git a/api/turing/api/deployment_controller.go b/api/turing/api/deployment_controller.go
@@ -10,10 +10,8 @@ import (
 	merlin "github.com/caraml-dev/merlin/client"
 	mlp "github.com/caraml-dev/mlp/api/client"
 
-	"github.com/caraml-dev/turing/api/turing/log"
 	"github.com/caraml-dev/turing/api/turing/models"
 	"github.com/caraml-dev/turing/api/turing/service"
-	"github.com/caraml-dev/turing/api/turing/webhook"
 	"github.com/caraml-dev/turing/engines/experiment/manager"
 )
 
@@ -99,15 +97,6 @@ func (c RouterDeploymentController) deployOrRollbackRouter(
 
 		err = errors.New(strings.Join(errorStrings, ". "))
 
-		// call webhook for router un-deployment event
-		if errWebhook := c.webhookClient.TriggerWebhooks(
-			ctx, webhook.OnRouterUndeployed, routerVersion,
-		); errWebhook != nil {
-			log.Warnf(
-				"Error triggering webhook for event %s, router id: %d, router version id: %d, %v",
-				webhook.OnRouterUndeployed, router.ID, routerVersion.ID, errWebhook,
-			)
-		}
 		return err
 	}
 
@@ -125,16 +114,6 @@ func (c RouterDeploymentController) deployOrRollbackRouter(
 			eventsCh.Write(models.NewInfoEvent(models.EventStageUndeployingPreviousVersion,
 				"successfully undeployed previously deployed version %d",
 				router.CurrRouterVersion.Version))
-
-			// call webhook for router un-deployment event
-			if errWebhook := c.webhookClient.TriggerWebhooks(
-				ctx, webhook.OnRouterUndeployed, currVersion,
-			); errWebhook != nil {
-				log.Warnf(
-					"Error triggering webhook for event %s, router id: %d, router version id: %d, %v",
-					webhook.OnRouterUndeployed, router.ID, currVersion.ID, errWebhook,
-				)
-			}
 		}
 	}
 
@@ -300,16 +279,6 @@ func (c RouterDeploymentController) deployRouterVersion(
 	routerVersion.Status = models.RouterVersionStatusDeployed
 	_, err = c.RouterVersionsService.Save(routerVersion)
 
-	// call webhook for router deployment event
-	if errWebhook := c.webhookClient.TriggerWebhooks(
-		ctx, webhook.OnRouterDeployed, routerVersion,
-	); errWebhook != nil {
-		log.Warnf(
-			"Error triggering webhook for event %s, router id: %d, router version id: %d, %v",
-			webhook.OnRouterDeployed, router.ID, routerVersion.ID, errWebhook,
-		)
-	}
-
 	return endpoint, err
 }
 

diff --git a/api/turing/api/deployment_controller_test.go b/api/turing/api/deployment_controller_test.go
@@ -14,7 +14,6 @@ import (
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 
-	"github.com/caraml-dev/turing/api/turing/webhook"
 	webhookMock "github.com/caraml-dev/turing/api/turing/webhook/mocks"
 
 	"github.com/caraml-dev/turing/api/turing/config"
@@ -150,12 +149,6 @@ func TestDeployVersionSuccess(t *testing.T) {
 	cs := &mocks.CryptoService{}
 	cs.On("Decrypt", testPassKey).Return(testDecPassKey, nil)
 
-	// Mock webhook service
-	webhookSvc := &webhookMock.Client{}
-	webhookSvc.On(
-		"TriggerWebhooks", mock.Anything, webhook.OnRouterDeployed, mock.Anything,
-	).Return(nil)
-
 	// Run tests and validate
 	for name, data := range tests {
 		t.Run(name, func(t *testing.T) {
@@ -198,7 +191,6 @@ func TestDeployVersionSuccess(t *testing.T) {
 						CryptoService:         cs,
 						ExperimentsService:    exps,
 					},
-					webhookClient: webhookSvc,
 				},
 			}
 
@@ -308,15 +300,6 @@ func TestRollbackVersionSuccess(t *testing.T) {
 	exps := &mocks.ExperimentsService{}
 	exps.On("IsClientSelectionEnabled", "nop").Return(false, nil)
 
-	// Mock webhook service
-	webhookSvc := &webhookMock.Client{}
-	webhookSvc.On(
-		"TriggerWebhooks", mock.Anything, webhook.OnRouterDeployed, mock.Anything,
-	).Return(nil)
-	webhookSvc.On(
-		"TriggerWebhooks", mock.Anything, webhook.OnRouterUndeployed, mock.Anything,
-	).Return(nil)
-
 	// Create test controller
 	ctrl := RouterDeploymentController{
 		BaseController{
@@ -328,7 +311,7 @@ func TestRollbackVersionSuccess(t *testing.T) {
 				EventService:          es,
 				ExperimentsService:    exps,
 			},
-			webhookClient: webhookSvc,
+			webhookClient: &webhookMock.Client{},
 		},
 	}
 

diff --git a/api/turing/api/router_versions_api.go b/api/turing/api/router_versions_api.go
@@ -2,6 +2,7 @@ package api
 
 import (
 	"fmt"
+	"github.com/caraml-dev/turing/api/turing/webhook"
 	"net/http"
 
 	"github.com/caraml-dev/turing/api/turing/service"
@@ -41,11 +42,15 @@ func (c RouterVersionsController) ListRouterVersions(
 // CreateRouterVersion creates a router version from the provided configuration. If no router exists
 // within the provided project with the provided id, this method will throw an error.
 // If the update is valid, a new RouterVersion will be created but NOT deployed.
-func (c RouterVersionsController) CreateRouterVersion(_ *http.Request, vars RequestVars, body interface{}) *Response {
+func (c RouterVersionsController) CreateRouterVersion(req *http.Request, vars RequestVars, body interface{}) *Response {
 	// Parse request vars
-	var errResp *Response
-	var router *models.Router
-	var project *mlp.Project
+	var (
+		ctx     = req.Context()
+		errResp *Response
+		router  *models.Router
+		project *mlp.Project
+	)
+
 	if project, errResp = c.getProjectFromRequestVars(vars); errResp != nil {
 		return errResp
 	}
@@ -79,6 +84,16 @@ func (c RouterVersionsController) CreateRouterVersion(_ *http.Request, vars Requ
 		return InternalServerError("unable to create router version", err.Error())
 	}
 
+	// call webhook for router version creation event
+	if errWebhook := c.webhookClient.TriggerWebhooks(
+		ctx, webhook.OnRouterVersionCreated, routerVersion,
+	); errWebhook != nil {
+		log.Warnf(
+			"Error triggering webhook for event %s, router id: %d, router version id: %d, %v",
+			webhook.OnRouterVersionCreated, router.ID, routerVersion.ID, errWebhook,
+		)
+	}
+
 	return Ok(routerVersion)
 }
 
@@ -100,14 +115,18 @@ func (c RouterVersionsController) GetRouterVersion(
 
 // DeleteRouterVersion deletes the config for the given version number.
 func (c RouterVersionsController) DeleteRouterVersion(
-	_ *http.Request,
+	req *http.Request,
 	vars RequestVars,
 	_ interface{},
 ) *Response {
 	// Parse request vars
-	var errResp *Response
-	var router *models.Router
-	var routerVersion *models.RouterVersion
+	var (
+		ctx           = req.Context()
+		errResp       *Response
+		router        *models.Router
+		routerVersion *models.RouterVersion
+	)
+
 	if router, errResp = c.getRouterFromRequestVars(vars); errResp != nil {
 		return errResp
 	}
@@ -130,20 +149,34 @@ func (c RouterVersionsController) DeleteRouterVersion(
 	if err != nil {
 		return InternalServerError("unable to delete router version", err.Error())
 	}
+
+	// call webhook for router version deletion event
+	if errWebhook := c.webhookClient.TriggerWebhooks(
+		ctx, webhook.OnRouterVersionDeleted, routerVersion,
+	); errWebhook != nil {
+		log.Warnf(
+			"Error triggering webhook for event %s, router id: %d, router version id: %d, %v",
+			webhook.OnRouterVersionDeleted, router.ID, routerVersion.ID, errWebhook,
+		)
+	}
 	return Ok(map[string]int{"router_id": int(router.ID), "version": int(routerVersion.Version)})
 }
 
 // DeployRouterVersion deploys the given router version into the associated kubernetes cluster
 func (c RouterVersionsController) DeployRouterVersion(
-	_ *http.Request,
+	req *http.Request,
 	vars RequestVars,
 	_ interface{},
 ) *Response {
 	// Parse request vars
-	var errResp *Response
-	var project *mlp.Project
-	var router *models.Router
-	var routerVersion *models.RouterVersion
+	var (
+		ctx           = req.Context()
+		errResp       *Response
+		project       *mlp.Project
+		router        *models.Router
+		routerVersion *models.RouterVersion
+	)
+
 	if project, errResp = c.getProjectFromRequestVars(vars); errResp != nil {
 		return errResp
 	}
@@ -173,6 +206,16 @@ func (c RouterVersionsController) DeployRouterVersion(
 			log.Errorf("Error deploying router version %s:%s:%d: %v",
 				project.Name, router.Name, routerVersion.Version, err)
 		}
+
+		// call webhook for router version deployment event
+		if errWebhook := c.webhookClient.TriggerWebhooks(
+			ctx, webhook.OnRouterVersionDeployed, routerVersion,
+		); errWebhook != nil {
+			log.Warnf(
+				"Error triggering webhook for event %s, router id: %d, router version id: %d, %v",
+				webhook.OnRouterVersionDeployed, router.ID, routerVersion.ID, errWebhook,
+			)
+		}
 	}()
 
 	return Accepted(map[string]int{

diff --git a/api/turing/api/router_versions_api_test.go b/api/turing/api/router_versions_api_test.go
@@ -2,6 +2,9 @@ package api
 
 import (
 	"errors"
+	"github.com/caraml-dev/turing/api/turing/webhook"
+	webhookMock "github.com/caraml-dev/turing/api/turing/webhook/mocks"
+	"net/http"
 	"testing"
 
 	merlin "github.com/caraml-dev/merlin/client"
@@ -127,6 +130,10 @@ func TestCreateRouterVersion(t *testing.T) {
 	routerVersionSvc := &mocks.RouterVersionsService{}
 	routerVersionSvc.On("Save", routerVersion).Return(routerVersion, nil)
 
+	// Webhook service
+	webhookSvc := &webhookMock.Client{}
+	webhookSvc.On("TriggerWebhooks", mock.Anything, webhook.OnRouterVersionCreated, mock.Anything).Return(nil)
+
 	// Define tests
 	tests := map[string]struct {
 		vars     RequestVars
@@ -187,11 +194,12 @@ func TestCreateRouterVersion(t *testing.T) {
 							RouterVersionsService: routerVersionSvc,
 							RouterDefaults:        &config.RouterDefaults{},
 						},
+						webhookClient: webhookSvc,
 					},
 				},
 			}
 			// Run test method and validate
-			response := ctrl.CreateRouterVersion(nil, data.vars, data.body)
+			response := ctrl.CreateRouterVersion(&http.Request{}, data.vars, data.body)
 			assert.Equal(t, data.expected, response)
 		})
 	}
@@ -331,6 +339,10 @@ func TestDeleteRouterVersion(t *testing.T) {
 		On("FindByID", models.ID(2)).
 		Return(router2, nil)
 
+	// Webhook service
+	webhookSvc := &webhookMock.Client{}
+	webhookSvc.On("TriggerWebhooks", mock.Anything, webhook.OnRouterVersionDeleted, mock.Anything).Return(nil)
+
 	// Define tests
 	tests := map[string]struct {
 		vars     RequestVars
@@ -404,11 +416,12 @@ func TestDeleteRouterVersion(t *testing.T) {
 							RoutersService:        routerSvc,
 							RouterVersionsService: routerVersionSvc,
 						},
+						webhookClient: webhookSvc,
 					},
 				},
 			}
 			// Run test method and validate
-			response := ctrl.DeleteRouterVersion(nil, data.vars, nil)
+			response := ctrl.DeleteRouterVersion(&http.Request{}, data.vars, nil)
 			assert.Equal(t, data.expected, response)
 		})
 	}
@@ -522,6 +535,10 @@ func TestDeployRouterVersion(t *testing.T) {
 		On("FindByRouterIDAndVersion", models.ID(4), uint(4)).
 		Return(routerVersion4, nil)
 
+	// Webhook service
+	webhookSvc := &webhookMock.Client{}
+	webhookSvc.On("TriggerWebhooks", mock.Anything, webhook.OnRouterVersionDeployed, mock.Anything).Return(nil)
+
 	// Define tests
 	tests := map[string]struct {
 		vars     RequestVars
@@ -586,11 +603,12 @@ func TestDeployRouterVersion(t *testing.T) {
 							RouterVersionsService: routerVersionSvc,
 							RouterDefaults:        &config.RouterDefaults{},
 						},
+						webhookClient: webhookSvc,
 					},
 				},
 			}
 			// Run test method and validate
-			response := ctrl.DeployRouterVersion(nil, data.vars, nil)
+			response := ctrl.DeployRouterVersion(&http.Request{}, data.vars, nil)
 			assert.Equal(t, data.expected, response)
 		})
 	}