[TT-11966/TT-12064] implement OAS webhooks events by jeffy-mathew · Pull Request #6258 · TykTechnologies/tyk

jeffy-mathew · 2024-05-03T15:02:49Z

User description

implement OAS webhooks events
implement webhook disabled flag
refactor code to support events

Description

Related Issue

Parent: https://tyktech.atlassian.net/browse/TT-11966
Subtask: https://tyktech.atlassian.net/browse/TT-12064

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

I ensured that the documentation is up to date
I explained why this PR updates go.mod in detail with reasoning why it's required
I would like a code coverage CI quality gate exception and have explained why

PR Type

enhancement, bug_fix

Description

Refactored event handling to use new pkg/event package, enhancing maintainability and clarity.
Introduced WebHookHandlerConf with a Decode method to facilitate webhook configuration.
Added methods in apidef/oas/server.go to handle event configurations, improving OAS support.
Updated all relevant tests to align with the new event handling approach.
Added new configurations and methods in config.go to utilize new event types, improving system configurability.

Changes walkthrough 📝

Relevant files

Enhancement

6 files

api_definitions.go `Refactor Event Handling and Add Webhook Configuration` apidef/api_definitions.go Introduced new types and constants for event handling using the `pkg/event` package. Refactored `EventHandlerTriggerConfig` and `EventHandlerMetaConfig` to use new event types. Added `WebHookHandlerConf` struct for webhook configuration. Added `Decode` method to `WebHookHandlerConf` for decoding configuration.	+39/-4
migration.go `Implement Event Handler Disabling in Migration` apidef/migration.go Added method `setEventHandlersDisabledFlags` to disable event handlers during migration.	+12/-0
server.go `Add Event Configuration Handling in OAS Server` apidef/oas/server.go Added `Events` field to `Server` struct for event configuration. Implemented methods to fill and extract event configurations to/from `apidef.APIDefinition`.	+150/-0
config.go `Update Configurations to Use New Event Types` config/config.go Updated event-related configurations to use new event types from `pkg/event`. Added method `SetEventTriggers` for setting event triggers.	+8/-5
event_system.go `Refactor Gateway Event System to Use New Event Types` gateway/event_system.go Refactored event system to use new event types and handler names from `pkg/event`.	+45/-23
event.go `Introduce New Event Package with Constants and Types` pkg/event/event.go Introduced new package `pkg/event` with constants and types for event handling.	+62/-0

Tests

5 files

migration_test.go `Update Migration Tests for Event Handler Flags` apidef/migration_test.go Updated tests to include checks for disabled event handlers.	+28/-0
linter_test.go `Extend OAS Linter Tests for Event Handling` apidef/oas/linter_test.go Extended linting tests to include event settings verification.	+9/-0
server_test.go `Comprehensive Tests for Event Configuration in OAS Server` apidef/oas/server_test.go Added comprehensive tests for event configuration extraction and filling.	+213/-0
config_test.go `Update Configuration Tests for New Event Types` config/config_test.go Updated configuration tests to reflect new event types.	+2/-2
event_system_test.go `Update Event System Tests for New Event Types` gateway/event_system_test.go Updated event system tests to use new event types and handler names.	+12/-10

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions · 2024-05-03T15:03:33Z

API Changes

--- prev.txt	2024-05-07 14:32:20.760436472 +0000
+++ current.txt	2024-05-07 14:32:17.900402827 +0000
@@ -1806,9 +1806,11 @@
 	Method string `bson:"method" json:"method"`
 }
 
-type TykEvent string // A type so we can ENUM event types easily, e.g. EventQuotaExceeded
+type TykEvent = event.Event
+    TykEvent is an alias maintained for backwards compatibility.
 
-type TykEventHandlerName string // A type for handler codes in API definitions
+type TykEventHandlerName = event.HandlerName
+    TykEventHandlerName is an alias maintained for backwards compatibility.
 
 type UDGGlobalHeader struct {
 	Key   string `bson:"key" json:"key"`
@@ -1947,6 +1949,29 @@
 	ProxyOnError         bool       `bson:"proxy_on_error" json:"proxy_on_error"`
 }
 
+type WebHookHandlerConf struct {
+	// Disabled enables/disables this webhook.
+	Disabled bool `bson:"disabled" json:"disabled"`
+	// ID optional ID of the webhook, to be used in pro mode.
+	ID string `bson:"id" json:"id"`
+	// Name is the name of webhook.
+	Name string `bson:"name" json:"name"`
+	// The method to use for the webhook.
+	Method string `bson:"method" json:"method"`
+	// The target path on which to send the request.
+	TargetPath string `bson:"target_path" json:"target_path"`
+	// The template to load in order to format the request.
+	TemplatePath string `bson:"template_path" json:"template_path"`
+	// Headers to set when firing the webhook.
+	HeaderList map[string]string `bson:"header_map" json:"header_map"`
+	// The cool-down for the event so it does not trigger again (in seconds).
+	EventTimeout int64 `bson:"event_timeout" json:"event_timeout"`
+}
+    WebHookHandlerConf holds configuration related to webhook event handler.
+
+func (w *WebHookHandlerConf) Scan(in any) error
+    Scan scans WebHookHandlerConf from `any` in.
+
 # Package: ./apidef/adapter
 
 package adapter // import "github.com/TykTechnologies/tyk/apidef/adapter"
@@ -2908,6 +2933,43 @@
 func (et *EnforceTimeout) Fill(meta apidef.HardTimeoutMeta)
     Fill fills *EnforceTimeout from apidef.HardTimeoutMeta.
 
+type Event struct {
+	// Enabled enables the event handler.
+	Enabled bool `json:"enabled" bson:"enabled"`
+	// Type specifies the TykEvent that should trigger the event handler.
+	Type event.Event `json:"type" bson:"type"`
+	// Action specifies the action to be taken on the event trigger.
+	Action event.Action `json:"action" bson:"action"`
+	// ID is the ID of event handler in storage.
+	ID string `json:"id,omitempty" bson:"id,omitempty"`
+	// Name is the name of event handler
+	Name string `json:"name,omitempty" bson:"name,omitempty"`
+
+	Webhook WebhookEvent `bson:"-" json:"-"`
+}
+    Event holds information about individual event to be configured on the API.
+
+func (e *Event) GetWebhookConf() (map[string]interface{}, error)
+    GetWebhookConf converts Event.WebhookEvent to map[string]interface{}
+    with apidef.WebHookHandlerConf structure for classic API definition
+    compatibility.
+
+func (e *Event) MarshalJSON() ([]byte, error)
+    MarshalJSON marshals Event as per Tyk OAS API definition contract.
+
+func (e *Event) UnmarshalJSON(in []byte) error
+    UnmarshalJSON unmarshals Event as per Tyk OAS API definition contract.
+
+type Events []Event
+    Events holds the list of events to be processed for the API.
+
+func (e *Events) ExtractTo(api *apidef.APIDefinition)
+    ExtractTo extracts events to apidef.APIDefinition.
+
+func (e *Events) Fill(api apidef.APIDefinition)
+    Fill fills Events from classic API definition. Currently only webhook events
+    are supported.
+
 type ExternalOAuth struct {
 	Enabled     bool `bson:"enabled" json:"enabled"` // required
 	AuthSources `bson:",inline" json:",inline"`
@@ -3827,6 +3889,11 @@
 	//
 	// Tyk classic API definition: `detailed_tracing`
 	DetailedTracing *DetailedTracing `bson:"detailedTracing,omitempty" json:"detailedTracing,omitempty"`
+
+	// Events contains the configuration related to Tyk Events.
+	//
+	// Tyk classic API definition: `event_handlers`
+	Events Events `bson:"events,omitempty" json:"events,omitempty"`
 }
     Server contains the configuration that sets Tyk up to receive requests from
     the client applications.
@@ -4345,6 +4412,15 @@
     It is implemented to facilitate a smooth migration from deprecated fields
     that were previously used to represent the same data.
 
+type WebhookEvent struct {
+	URL          string            `json:"url" bson:"url"`
+	Method       string            `json:"method" bson:"method"`
+	Timeout      int64             `json:"timeout" bson:"timeout"`
+	BodyTemplate string            `json:"bodyTemplate,omitempty" bson:"bodyTemplate,omitempty"`
+	Headers      map[string]string `json:"headers,omitempty" bson:"headers,omitempty"`
+}
+    WebhookEvent stores the core information about a webhook event.
+
 type XTykAPIGateway struct {
 	// Info contains the main metadata for the API definition.
 	Info Info `bson:"info" json:"info"` // required
@@ -5299,6 +5375,7 @@
 func (c *Config) LoadIgnoredIPs()
 
 func (c *Config) SetEventTriggers(eventTriggers map[apidef.TykEvent][]TykEventHandler)
+    SetEventTriggers sets events for backwards compatibility
 
 func (c *Config) StoreAnalytics(ip string) bool
 
@@ -6793,26 +6870,40 @@
     other types. This is a go <1.13 cludge.
 
 const (
-	EventQuotaExceeded        apidef.TykEvent = "QuotaExceeded"
-	EventRateLimitExceeded    apidef.TykEvent = "RatelimitExceeded"
-	EventAuthFailure          apidef.TykEvent = "AuthFailure"
-	EventKeyExpired           apidef.TykEvent = "KeyExpired"
-	EventVersionFailure       apidef.TykEvent = "VersionFailure"
-	EventOrgQuotaExceeded     apidef.TykEvent = "OrgQuotaExceeded"
-	EventOrgRateLimitExceeded apidef.TykEvent = "OrgRateLimitExceeded"
-	EventTriggerExceeded      apidef.TykEvent = "TriggerExceeded"
-	EventBreakerTriggered     apidef.TykEvent = "BreakerTriggered"
-	EventBreakerTripped       apidef.TykEvent = "BreakerTripped"
-	EventBreakerReset         apidef.TykEvent = "BreakerReset"
-	EventHOSTDOWN             apidef.TykEvent = "HostDown"
-	EventHOSTUP               apidef.TykEvent = "HostUp"
-	EventTokenCreated         apidef.TykEvent = "TokenCreated"
-	EventTokenUpdated         apidef.TykEvent = "TokenUpdated"
-	EventTokenDeleted         apidef.TykEvent = "TokenDeleted"
+	// EventQuotaExceeded is an alias maintained for backwards compatibility.
+	EventQuotaExceeded = event.QuotaExceeded
+	// EventRateLimitExceeded is an alias maintained for backwards compatibility.
+	EventRateLimitExceeded = event.RateLimitExceeded
+
+	// EventAuthFailure is an alias maintained for backwards compatibility.
+	EventAuthFailure = event.AuthFailure
+	// EventKeyExpired is an alias maintained for backwards compatibility.
+	EventKeyExpired = event.KeyExpired
+	// EventVersionFailure is an alias maintained for backwards compatibility.
+	EventVersionFailure = event.VersionFailure
+	// EventOrgQuotaExceeded is an alias maintained for backwards compatibility.
+	EventOrgQuotaExceeded = event.OrgQuotaExceeded
+	// EventOrgRateLimitExceeded is an alias maintained for backwards compatibility.
+	EventOrgRateLimitExceeded = event.OrgRateLimitExceeded
+	// EventTriggerExceeded is an alias maintained for backwards compatibility.
+	EventTriggerExceeded = event.TriggerExceeded
+	// EventBreakerTriggered is an alias maintained for backwards compatibility.
+	EventBreakerTriggered = event.BreakerTriggered
+	// EventBreakerTripped is an alias maintained for backwards compatibility.
+	EventBreakerTripped = event.BreakerTripped
+	// EventBreakerReset is an alias maintained for backwards compatibility.
+	EventBreakerReset = event.BreakerReset
+	// EventHOSTDOWN is an alias maintained for backwards compatibility.
+	EventHOSTDOWN = event.HostDown
+	// EventHOSTUP is an alias maintained for backwards compatibility.
+	EventHOSTUP = event.HostUp
+	// EventTokenCreated is an alias maintained for backwards compatibility.
+	EventTokenCreated = event.TokenCreated
+	// EventTokenUpdated is an alias maintained for backwards compatibility.
+	EventTokenUpdated = event.TokenUpdated
+	// EventTokenDeleted is an alias maintained for backwards compatibility.
+	EventTokenDeleted = event.TokenDeleted
 )
-    Register new event types here, the string is the code used to hook at the
-    Api Deifnititon JSON/BSON level
-
 const (
 	MsgAuthFieldMissing                        = "Authorization field missing"
 	MsgApiAccessDisallowed                     = "Access to this API has been disallowed"
@@ -6900,11 +6991,18 @@
 const CoProcessDefaultKeyPrefix = "coprocess-data:"
     CoProcessDefaultKeyPrefix is used as a key prefix for this CP.
 
-const EH_CoProcessHandler apidef.TykEventHandlerName = "cp_dynamic_handler"
-    Constant for event system.
+const EH_CoProcessHandler = event.CoProcessHandler
+    EH_CoProcessHandler is used for event system, maintained here for backwards
+    compatibility.
 
-const EH_JSVMHandler apidef.TykEventHandlerName = "eh_dynamic_handler"
-const EH_LogHandler apidef.TykEventHandlerName = "eh_log_handler"
+const EH_JSVMHandler = event.JSVMHandler
+    EH_JSVMHandler is aliased for backwards compatibility.
+
+const (
+	// EH_LogHandler is an alias maintained for backwards compatibility.
+	// It is used to register log handler on an event.
+	EH_LogHandler = event.LogHandler
+)
     The name for event handlers as defined in the API Definition JSON/BSON
     format
 
@@ -10025,8 +10123,9 @@
 	WH_DELETE WebHookRequestMethod = "DELETE"
 	WH_PATCH  WebHookRequestMethod = "PATCH"
 
-	// Define the Event Handler name so we can register it
-	EH_WebHook apidef.TykEventHandlerName = "eh_web_hook_handler"
+	// EH_WebHook is an alias maintained for backwards compatibility.
+	// it is the handler to register a webhook event.
+	EH_WebHook = event.WebHookHandler
 )
 type XPathExtractor struct {
 	BaseExtractor

github-actions · 2024-05-06T09:31:36Z

PR Description updated to latest commit (50963a1)

github-actions · 2024-05-06T09:32:19Z

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, due to the extensive changes across multiple files involving event handling, webhook configurations, and API definitions. The PR involves refactoring and introducing new structures which require careful review to ensure compatibility and correctness.
🧪 Relevant tests	Yes
⚡ Possible issues	Possible Bug: The new `WebHookHandlerConf` structure includes a `Decode` method which directly marshals and unmarshals JSON. This could lead to issues if the input map does not perfectly align with the expected structure, potentially leading to data loss or misconfiguration.
⚡ Possible issues	Compatibility Issue: The changes in event and handler names need thorough testing to ensure they do not break existing configurations, especially since aliases are maintained for backward compatibility.
🔒 Security concerns	No

Code feedback:

relevant file	apidef/api_definitions.go
suggestion	Consider adding error handling for the `Decode` method in `WebHookHandlerConf` to manage unexpected input formats gracefully. This method directly marshals and unmarshals JSON without any checks on the input map, which might lead to runtime errors if the map contains unexpected data. [important]
relevant line	func (w *WebHookHandlerConf) Decode(in map[string]interface{}) error {

relevant file	gateway/event_handler_webhooks.go
suggestion	Implement logging for the `Init` method in `WebHookHandler` when skipping disabled webhooks. This will help in debugging and understanding the flow of webhook handling, especially in production environments where disabled webhooks might lead to confusion. [medium]
relevant line	if whConf.Disabled {

relevant file	apidef/oas/server.go
suggestion	In the `ExtractTo` method of `Events`, ensure that the `Events` field is properly initialized before use to avoid nil pointer dereferences when no events are configured. [important]
relevant line	if s.Events == nil {

relevant file	gateway/event_system.go
suggestion	Update the `fireEvent` function to handle nil or empty handler slices gracefully. This prevents potential runtime panics when an event is fired without any configured handlers. [medium]
relevant line	func fireEvent(name event.Event, meta interface{}, handlers map[event.Event][]config.TykEventHandler) {

github-actions · 2024-05-06T09:34:05Z

PR Code Suggestions ✨

Category	Suggestions
Enhancement	Validate the HTTP method in the webhook configuration. It is recommended to validate the `Method` field in the `WebHookHandlerConf` struct to ensure it only accepts valid HTTP methods. This can prevent runtime errors or misconfigurations. apidef/api_definitions.go [1419] -Method string `bson:"method" json:"method"` +Method HttpMethod `bson:"method" json:"method"`
	Add error logging in the webhook configuration decoding. To improve error handling, consider logging the error before returning it in the `Decode` method of the `WebHookHandlerConf` struct. apidef/api_definitions.go [1434] +log.Errorf("Error unmarshalling data: %v", err) return err
	Add detailed logging for disabled webhook initialization. Ensure that the `WebHookHandler` initialization logs the disabling of a webhook for better traceability and debugging. gateway/event_handler_webhooks.go [77-82] if whConf.Disabled { log.WithFields(logrus.Fields{ "prefix": "webhooks", - }).Infof("skipping disabled webhook %s", whConf.Name) + "webhook_name": whConf.Name, + }).Info("Webhook is disabled and will be skipped.") return nil }
	Improve the constant name for clarity and readability. Use a more descriptive constant name for `EH_CoProcessHandler` to clarify its purpose in the system. gateway/coprocess_events.go [12] -const EH_CoProcessHandler = event.CoProcessHandler +const EventCoProcessHandler = event.CoProcessHandler
	Rename the event to clearly indicate it can represent multiple states. To avoid potential confusion and maintain clarity, consider renaming the `BreakerTriggered` event to more clearly reflect that it can represent two distinct states (tripped and reset). This change would help developers understand the dual nature of this event without needing to refer to the comments or documentation. pkg/event/event.go [25] -BreakerTriggered Event = "BreakerTriggered" +BreakerStatusChanged Event = "BreakerStatusChanged"
Bug	Initialize map to prevent nil map assignment errors. Consider initializing `HandlerMeta` map in the `EventHandlerTriggerConfig` struct to avoid potential nil map assignment errors. apidef/api_definitions.go [468] HandlerMeta map[string]interface{} `bson:"handler_meta" json:"handler_meta"` +func init() { + HandlerMeta = make(map[string]interface{}) +}
Bug	Fix typo in JSON tag to ensure correct serialization. Correct the typo in the JSON tag from `event_trigers_defunct` to `event_triggers_defunct` to ensure the JSON serialization/deserialization works as expected. config/config.go [1007] -EventTriggers map[event.Event][]TykEventHandler `json:"event_trigers_defunct"` // Deprecated: Config.GetEventTriggers instead. +EventTriggers map[event.Event][]TykEventHandler `json:"event_triggers_defunct"` // Deprecated: Config.GetEventTriggers instead.
Possible issue	Ensure thread safety when modifying shared data structures. To ensure thread safety and avoid potential race conditions, consider using a mutex or similar synchronization technique when modifying the `HandlerMeta` map in the `setEventHandlersDisabledFlags` method. apidef/migration.go [469] +a.mutex.Lock() a.EventHandlers.Events[k][i].HandlerMeta["disabled"] = true +a.mutex.Unlock()
Maintainability	Refactor repeated logic into a method for setting handler configurations. For better maintainability and to avoid duplication, consider creating a method to handle the repeated logic of setting `Handler` and `HandlerMeta` in `EventHandlerTriggerConfig`. apidef/api_definitions.go [467-468] -Handler event.HandlerName `bson:"handler_name" json:"handler_name"` -HandlerMeta map[string]interface{} `bson:"handler_meta" json:"handler_meta"` +func (e *EventHandlerTriggerConfig) SetHandler(name event.HandlerName, meta map[string]interface{}) { + e.Handler = name + e.HandlerMeta = meta +}
	Reduce redundancy in test case setup by using a loop. Consider using a loop or a helper function to reduce the redundancy in the test case setup for webhook events. This will make the code cleaner and easier to maintain. apidef/oas/server_test.go [340-368] -{ - Enabled: true, - Type: tykevent.QuotaExceeded, - Action: tykevent.WebhookAction, - ID: "random-id", - WebhookCore: WebhookCore{ - Name: "test-webhook", - URL: "https://webhook.site/uuid", - Headers: map[string]string{"Auth": "key"}, - BodyTemplate: "/path/to/template", - Timeout: 20, - Method: http.MethodPost, - }, -}, +for _, event := range []tykevent.Event{tykevent.QuotaExceeded, tykevent.RateLimitExceeded} { + testcases = append(testcases, struct{ + Enabled: true, + Type: event, + Action: tykevent.WebhookAction, + ID: "random-id", + WebhookCore: WebhookCore{ + Name: "test-webhook", + URL: "https://webhook.site/uuid", + Headers: map[string]string{"Auth": "key"}, + BodyTemplate: "/path/to/template", + Timeout: 20, + Method: http.MethodPost, + }, + }) +}
	Encapsulate event paths initialization in a function. Initialize `EventPaths` using a function to encapsulate the logic for setting up event handlers, improving code organization and maintainability. gateway/api_definition.go [383] -spec.EventPaths = make(map[event.Event][]config.TykEventHandler) +spec.EventPaths = initializeEventPaths(def.EventHandlers.Events)
	Consolidate constants into a single block for better organization. Consider consolidating the constants for event types into a single `const` block to improve readability and maintainability. gateway/event_system.go [22-60] const ( EH_LogHandler = event.LogHandler -) -const ( EventQuotaExceeded = apidef.TykEvent(event.QuotaExceeded) EventRateLimitExceeded = apidef.TykEvent(event.RateLimitExceeded) EventAuthFailure = apidef.TykEvent(event.AuthFailure) EventKeyExpired = apidef.TykEvent(event.KeyExpired) EventVersionFailure = apidef.TykEvent(event.VersionFailure) EventOrgQuotaExceeded = apidef.TykEvent(event.OrgQuotaExceeded) EventOrgRateLimitExceeded = apidef.TykEvent(event.OrgRateLimitExceeded) EventTriggerExceeded = apidef.TykEvent(event.TriggerExceeded) EventBreakerTriggered = apidef.TykEvent(event.BreakerTriggered) EventBreakerTripped = apidef.TykEvent(event.BreakerTripped) EventBreakerReset = apidef.TykEvent(event.BreakerReset) EventHOSTDOWN = apidef.TykEvent(event.HostDown) EventHOSTUP = apidef.TykEvent(event.HostUp) EventTokenCreated = apidef.TykEvent(event.TokenCreated) EventTokenUpdated = apidef.TykEvent(event.TokenUpdated) EventTokenDeleted = apidef.TykEvent(event.TokenDeleted) )
	Organize constants in a separate package or file for better manageability. For better maintainability and to avoid magic strings in your codebase, consider creating a separate package or file that contains all event and handler names as constants. This approach not only organizes the constants better but also makes it easier to manage them as the number of events and handlers grows. pkg/event/event.go [7-54] -QuotaExceeded Event = "QuotaExceeded" -RateLimitExceeded Event = "RatelimitExceeded" -AuthFailure Event = "AuthFailure" -KeyExpired Event = "KeyExpired" -VersionFailure Event = "VersionFailure" -OrgQuotaExceeded Event = "OrgQuotaExceeded" -OrgRateLimitExceeded Event = "OrgRateLimitExceeded" -TriggerExceeded Event = "TriggerExceeded" -BreakerTriggered Event = "BreakerTriggered" -BreakerTripped Event = "BreakerTripped" -BreakerReset Event = "BreakerReset" -HostDown Event = "HostDown" -HostUp Event = "HostUp" -TokenCreated Event = "TokenCreated" -TokenUpdated Event = "TokenUpdated" -TokenDeleted Event = "TokenDeleted" -LogHandler HandlerName = "eh_log_handler" -WebHookHandler HandlerName = "eh_web_hook_handler" -JSVMHandler HandlerName = "eh_dynamic_handler" -CoProcessHandler HandlerName = "cp_dynamic_handler" +// This would be in a separate constants package or file +const ( + QuotaExceeded Event = "QuotaExceeded" + RateLimitExceeded Event = "RateLimitExceeded" + AuthFailure Event = "AuthFailure" + KeyExpired Event = "KeyExpired" + VersionFailure Event = "VersionFailure" + OrgQuotaExceeded Event = "OrgQuotaExceeded" + OrgRateLimitExceeded Event = "OrgRateLimitExceeded" + TriggerExceeded Event = "TriggerExceeded" + BreakerStatusChanged Event = "BreakerStatusChanged" + BreakerTripped Event = "BreakerTripped" + BreakerReset Event = "BreakerReset" + HostDown Event = "HostDown" + HostUp Event = "HostUp" + TokenCreated Event = "TokenCreated" + TokenUpdated Event = "TokenUpdated" + TokenDeleted Event = "TokenDeleted" + LogHandler HandlerName = "eh_log_handler" + WebHookHandler HandlerName = "eh_web_hook_handler" + JSVMHandler HandlerName = "eh_dynamic_handler" + CoProcessHandler HandlerName = "cp_dynamic_handler" +)
	Add a prefix to handler constants to improve readability and identification. To improve the readability and maintainability of the `HandlerName` constants, consider adding a prefix or suffix to differentiate them clearly from other constants or identifiers in the system. This practice helps in quickly identifying the purpose of these constants in the code. pkg/event/event.go [46-53] -LogHandler HandlerName = "eh_log_handler" -WebHookHandler HandlerName = "eh_web_hook_handler" -JSVMHandler HandlerName = "eh_dynamic_handler" -CoProcessHandler HandlerName = "cp_dynamic_handler" +EventHandler_Log HandlerName = "eh_log_handler" +EventHandler_WebHook HandlerName = "eh_web_hook_handler" +EventHandler_JSVM HandlerName = "eh_dynamic_handler" +EventHandler_CoProcess HandlerName = "cp_dynamic_handler"
Best practice	Standardize the casing in constant values to improve consistency. Consider using a consistent naming convention for the `Event` constants. The constant `RateLimitExceeded` is inconsistently cased as "RatelimitExceeded" which might lead to confusion or errors when these constants are used elsewhere in the code. pkg/event/event.go [10] -RateLimitExceeded Event = "RatelimitExceeded" +RateLimitExceeded Event = "RateLimitExceeded"
Best practice	Implement tests to verify the integrity of event name constants. To ensure that the event names are used consistently and avoid typos, consider implementing a test that verifies the string values of these constants. This test would help maintain the integrity of the event system, especially as more events might be added or modified in the future. pkg/event/event.go [7-39] -QuotaExceeded Event = "QuotaExceeded" -RateLimitExceeded Event = "RatelimitExceeded" -AuthFailure Event = "AuthFailure" -KeyExpired Event = "KeyExpired" -VersionFailure Event = "VersionFailure" -OrgQuotaExceeded Event = "OrgQuotaExceeded" -OrgRateLimitExceeded Event = "OrgRateLimitExceeded" -TriggerExceeded Event = "TriggerExceeded" -BreakerTriggered Event = "BreakerTriggered" -BreakerTripped Event = "BreakerTripped" -BreakerReset Event = "BreakerReset" -HostDown Event = "HostDown" -HostUp Event = "HostUp" -TokenCreated Event = "TokenCreated" -TokenUpdated Event = "TokenUpdated" -TokenDeleted Event = "TokenDeleted" +// Example test in Go +func TestEventStringValues(t *testing.T) { + assert.Equal(t, "QuotaExceeded", string(QuotaExceeded)) + assert.Equal(t, "RateLimitExceeded", string(RateLimitExceeded)) + assert.Equal(t, "AuthFailure", string(AuthFailure)) + assert.Equal(t, "KeyExpired", string(KeyExpired)) + assert.Equal(t, "VersionFailure", string(VersionFailure)) + assert.Equal(t, "OrgQuotaExceeded", string(OrgQuotaExceeded)) + assert.Equal(t, "OrgRateLimitExceeded", string(OrgRateLimitExceeded)) + assert.Equal(t, "TriggerExceeded", string(TriggerExceeded)) + assert.Equal(t, "BreakerStatusChanged", string(BreakerStatusChanged)) + assert.Equal(t, "BreakerTripped", string(BreakerTripped)) + assert.Equal(t, "BreakerReset", string(BreakerReset)) + assert.Equal(t, "HostDown", string(HostDown)) + assert.Equal(t, "HostUp", string(HostUp)) + assert.Equal(t, "TokenCreated", string(TokenCreated)) + assert.Equal(t, "TokenUpdated", string(TokenUpdated)) + assert.Equal(t, "TokenDeleted", string(TokenDeleted)) +}

titpetric · 2024-05-06T09:56:15Z

+}
+
+// Decode decodes WebHookHandlerConf into map[string]interface{}.
+func (w *WebHookHandlerConf) Decode(in map[string]interface{}) error {


Can we use strong types and not resort to interfaces? Could the argument just be any, or do we need the map[string] semantics?

any could be used, but resorting to strong types wouldn't be possible atm since that's gonna increase the scope.

- implement webhook disabled flag

github-actions · 2024-05-06T13:42:19Z

💥 CI tests failed 🙈