APIGOV-33010 provisioning webhook dispatch - #1097
Conversation
| } | ||
| if action == proto.Event_SUBRESOURCEUPDATED && event.Metadata.GetSubresource() == defs.XWebhookDetails { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Just a thought here, would like more opinions....
Considering we now can have handlers for the same type with different subresouces if event types specified, do we want to consider moving new actions like this to their own handler?
Another thought. Since the pattern is pretty equal, could all webhook subres handling be in the same handler amongst the provisioning resrouces?
Finally, should this check also validate that the webhook config is actually set on the handler?
There was a problem hiding this comment.
Depending on the responsibilities we want the webhook to handle, a new handler for each resource might be necessary. Otherwise, a separate handler for the webhook subresource only might be a good idea
| func WithAccessRequestProvisioningWebhook(cfg config.ProvisioningWebhookEndpointConfig, client api.Client) func(c *accessRequestHandler) { | ||
| return func(c *accessRequestHandler) { | ||
| c.webhookCfg = cfg | ||
| c.webhookClient = client |
There was a problem hiding this comment.
Is this client a separate client from the one that is added to the handler already? Could the client already on the handler be updated to support both?
There was a problem hiding this comment.
I would not extend the handler client with webhook dispatch functionality because of the different purposes these two client have
| func (h *accessRequestHandler) onPending(ctx context.Context, ar *management.AccessRequest, mar *apiv1.ResourceInstance) *management.AccessRequest { | ||
| log := getLoggerFromContext(ctx) | ||
|
|
||
| if h.webhookCfg != nil && h.webhookCfg.IsConfigured() && webhookDispatchedFor(ar, webhookOperationProvision) { |
There was a problem hiding this comment.
does it makes sense to use a default webhook config on the handler that would result in IsConfigured returning false instead of having to check nil?
2 participants
No description provided.