Adding notification controller events

weaveworks / pipeline-controller

This repository contains the Pipeline CRD and associated controller for Weave GitOps Enterprise.

1 stars 4 forks source link

Adding notification controller events #56

Closed luizbafilho closed 1 year ago

luizbafilho commented 1 year ago

closes: https://github.com/weaveworks/pipeline-controller/issues/51

luizbafilho commented 1 year ago

I'm for making this a real strategy type instead of always emitting events towards n-c. This fits better to the principle of least surprise from the customer's view.

We can and should emit events towards the K8s API in any case, though.

there is a limitation on doing that, flux EventRecorder is a wrapper around the k8s one, so when we use it also sends the event to k8s API, so, we either send to both or none.

So, if we implement a strategy for that those events will only be published to k8s API when using the strategy. Not a big deal to me, but something to consider.

enekofb commented 1 year ago

I definitely like it

My 2cts is that

it makes sense to publish the event independently of the strategy
the event semantics would be more DeploymentHappened than PromotePipeline so events (at least in my experience) notify something that happened not intent
in that sense, the promotions endpoint becomes an event ingestor (which might have an overlap with kube api ?)
regarding the strategy being mandatory = yep, we could just make it optional as having the semantics of some action that we would like wge to execute (if required)

Questions

@luizbafilho how notification controller would consume the event here generated and how that looks like?

makkes commented 1 year ago

I'm for making this a real strategy type instead of always emitting events towards n-c. This fits better to the principle of least surprise from the customer's view. We can and should emit events towards the K8s API in any case, though.

there is a limitation on doing that, flux EventRecorder is a wrapper around the k8s one, so when we use it also sends the event to k8s API, so, we either send to both or none.

This is not correct. Flux's EventRecorder doesn't send an event to the webhook if it's not configured.

makkes commented 1 year ago

the event semantics would be more DeploymentHappened than PromotePipeline so events (at least in my experience) notify something that happened not intent

I don't agree with that sentiment. The Webhook we implemented is very specifically one to be used for promoting applications to the next environment in line. Changing this to be a generic "something happened" webhook doesn't differentiate it in any way from the upstream n-c so users that need that more generic functionality can just use n-c, otherwise they'd use p-c.

luizbafilho commented 1 year ago

I'm for making this a real strategy type instead of always emitting events towards n-c. This fits better to the principle of least surprise from the customer's view. We can and should emit events towards the K8s API in any case, though.

there is a limitation on doing that, flux EventRecorder is a wrapper around the k8s one, so when we use it also sends the event to k8s API, so, we either send to both or none.

This is not correct. Flux's EventRecorder doesn't send an event to the webhook if it's not configured.

I see, but it always sends the event to kube API right? so, how can we avoid duplicated events if we always send the event to Kube API and also trigger flux EventRecorder within a strategy?

makkes commented 1 year ago

I see, but it always sends the event to kube API right?

Yes.

so, how can we avoid duplicated events if we always send the event to Kube API and also trigger flux EventRecorder within a strategy?

This is not necessary. We can just always use the EventRecord. We would just need to configure it according to the Pipeline spec.

luizbafilho commented 1 year ago

Design update: https://github.com/weaveworks/weave-gitops-private/pull/81/files