From 9eece205cfa1a4571db7e3d05a3308dfe92d55e6 Mon Sep 17 00:00:00 2001 From: ewezy Date: Tue, 5 Nov 2024 16:40:16 +0800 Subject: [PATCH] Refactor positioning of webhook calls --- api/turing/api/deployment_controller.go | 31 --------- api/turing/api/deployment_controller_test.go | 19 +----- api/turing/api/router_versions_api.go | 69 ++++++++++++++++---- api/turing/api/router_versions_api_test.go | 24 ++++++- api/turing/api/routers_api.go | 35 ++++++---- api/turing/api/routers_api_test.go | 1 + api/turing/webhook/webhook.go | 7 ++ 7 files changed, 108 insertions(+), 78 deletions(-) diff --git a/api/turing/api/deployment_controller.go b/api/turing/api/deployment_controller.go index f0bca2ed3..43517a318 100644 --- 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 index 487aab2c8..a9049d141 100644 --- 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 index 8d4c37447..b90b5d9d4 100644 --- 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 index 50cb0ee3b..ef723f3a5 100644 --- 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) }) } diff --git a/api/turing/api/routers_api.go b/api/turing/api/routers_api.go index 98fbc3146..d6c07d285 100644 --- a/api/turing/api/routers_api.go +++ b/api/turing/api/routers_api.go @@ -123,12 +123,6 @@ func (c RoutersController) CreateRouter( return InternalServerError("unable to create router", strings.Join(errorStrings, ". ")) } - // call webhook for router creation event - if errWebhook := c.webhookClient.TriggerWebhooks(ctx, webhook.OnRouterCreated, router); errWebhook != nil { - log.Warnf("Error triggering webhook for event %s, router id: %d, %v", - webhook.OnRouterCreated, router.ID, errWebhook) - } - // deploy the new version go func() { err := c.deployOrRollbackRouter(project, router, routerVersion) @@ -136,6 +130,12 @@ func (c RoutersController) CreateRouter( log.Errorf("Error deploying router %s:%s:%d: %v", project.Name, router.Name, routerVersion.Version, err) } + + // call webhook for router creation event + if errWebhook := c.webhookClient.TriggerWebhooks(ctx, webhook.OnRouterCreated, router); errWebhook != nil { + log.Warnf("Error triggering webhook for event %s, router id: %d, %v", + webhook.OnRouterCreated, router.ID, errWebhook) + } }() return Ok(router) @@ -196,12 +196,6 @@ func (c RoutersController) UpdateRouter(req *http.Request, vars RequestVars, bod return InternalServerError("unable to update router", err.Error()) } - // call webhook for router update event - if errWebhook := c.webhookClient.TriggerWebhooks(ctx, webhook.OnRouterUpdated, router); errWebhook != nil { - log.Warnf("Error triggering webhook for event %s, router id: %d, %v", - webhook.OnRouterUpdated, router.ID, errWebhook) - } - // Deploy the new version go func() { err := c.deployOrRollbackRouter(project, router, routerVersion) @@ -211,6 +205,12 @@ func (c RoutersController) UpdateRouter(req *http.Request, vars RequestVars, bod } }() + // call webhook for router update event + if errWebhook := c.webhookClient.TriggerWebhooks(ctx, webhook.OnRouterUpdated, router); errWebhook != nil { + log.Warnf("Error triggering webhook for event %s, router id: %d, %v", + webhook.OnRouterUpdated, router.ID, errWebhook) + } + return Ok(router) } @@ -264,12 +264,13 @@ func (c RoutersController) DeleteRouter( // DeployRouter deploys the current version of the given router into the associated // kubernetes cluster. If there is no current version, an error is returned. func (c RoutersController) DeployRouter( - _ *http.Request, + req *http.Request, vars RequestVars, _ interface{}, ) *Response { // Parse request vars var ( + ctx = req.Context() errResp *Response project *mlp.Project router *models.Router @@ -310,6 +311,14 @@ func (c RoutersController) DeployRouter( log.Errorf("Error deploying router version %s:%s:%d: %v", project.Name, router.Name, routerVersion.Version, err) } + + // call webhook for router deployment event + if errWebhook := c.webhookClient.TriggerWebhooks(ctx, webhook.OnRouterDeployed, router); errWebhook != nil { + log.Warnf( + "Error triggering webhook for event %s, router id: %d, %v", + webhook.OnRouterDeployed, router.ID, errWebhook, + ) + } }() return Accepted(map[string]int{ diff --git a/api/turing/api/routers_api_test.go b/api/turing/api/routers_api_test.go index 1d964487f..a83a37f4d 100644 --- a/api/turing/api/routers_api_test.go +++ b/api/turing/api/routers_api_test.go @@ -763,6 +763,7 @@ func TestDeployRouter(t *testing.T) { // Webhook service webhookSvc := &webhookMock.Client{} + webhookSvc.On("TriggerWebhooks", mock.Anything, webhook.OnRouterDeployed, mock.Anything).Return(nil) // Define tests tests := map[string]struct { diff --git a/api/turing/webhook/webhook.go b/api/turing/webhook/webhook.go index 540851a7c..b27c77a4c 100644 --- a/api/turing/webhook/webhook.go +++ b/api/turing/webhook/webhook.go @@ -13,6 +13,10 @@ var ( OnRouterDeployed = webhooks.EventType("on-router-deployed") OnRouterUndeployed = webhooks.EventType("on-router-undeployed") + OnRouterVersionCreated = webhooks.EventType("on-router-version-created") + OnRouterVersionDeleted = webhooks.EventType("on-router-version-deleted") + OnRouterVersionDeployed = webhooks.EventType("on-router-version-deployed") + OnEnsemblerCreated = webhooks.EventType("on-ensembler-created") OnEnsemblerUpdated = webhooks.EventType("on-ensembler-updated") OnEnsemblerDeleted = webhooks.EventType("on-ensembler-deleted") @@ -23,6 +27,9 @@ var events = []webhooks.EventType{ OnRouterUpdated, OnRouterDeleted, OnRouterDeployed, + OnRouterVersionCreated, + OnRouterVersionDeleted, + OnRouterVersionDeployed, OnRouterUndeployed, OnEnsemblerCreated, OnEnsemblerUpdated,