Adds GitOps Toolkit EventRecorder to ArtifactGenerator reconciler#310
Adds GitOps Toolkit EventRecorder to ArtifactGenerator reconciler#310matheuscscp merged 2 commits intofluxcd:mainfrom
Conversation
Signed-off-by: Renato Vassão <renato.vassao@mindbodyonline.com>
|
Hi @renatovassaomb! Thanks for this PR 🙏 If you use your other account @renatovassao you will be able to use the CI without us allowing it to run. Also, I think we discussed in the dev meeting, this PR should not simply replace the current event recorder with the notification-controller one because the events that are currently sent are too much, it will be a lot of noise. We need to selectively send events to notification-controller for the most important ones, e.g. errors (make them aggregated i.e. do not send one error per source or per artifact), and an event when an artifact digest changes (should also be a single event for all artifacts whose digest changed). |
Sounds good, thanks for the insights @matheuscscp. I wanted to get this open to make sure I was on the right track, I'll keep working on it to meet these requirements. |
b46db4e to
184db32
Compare
|
@matheuscscp I updated the code to reflect the changes you specified. For regular events, a new function For error events, I noticed most of them end the reconciliation loop early, meaning they will fire only once. Is my assumption correct? |
matheuscscp
left a comment
There was a problem hiding this comment.
For error events, I noticed most of them end the reconciliation loop early, meaning they will fire only once.
Is my assumption correct?
Correct!
|
|
||
| for _, eaRef := range eaRefs { | ||
| if !oldObj.HasArtifactInInventory(eaRef.Name, eaRef.Namespace, eaRef.Digest) { | ||
| eaChanged = append(eaChanged, fmt.Sprintf("%s/%s", eaRef.Namespace, eaRef.Name)) |
There was a problem hiding this comment.
In source-controller events e.g. OCIRepository the digest is visible in the event sent to notification-controller
There was a problem hiding this comment.
Sounds good, adding the digest will make the event message a bit long, is that a problem?
> k get events --sort-by='.metadata.creationTimestamp' --field-selector involvedObject.kind=ArtifactGenerator | tail -n 5
29s Normal Ready artifactgenerator/my-app reconciliation succeeded, generated 2 artifact(s)
51s Normal Ready artifactgenerator/my-app external artifacts reconciled: default/my-app-composite2 (sha256:1aa3a5e94e257be1ef637d27e6fac4d850161dc20a2bd2327e2f2a5b3fdfafd5)
29s Normal Ready artifactgenerator/my-app ExternalArtifact/default/my-app-composite reconciled with revision latest@sha256:21f6a626fe6ad23fc6fb1c02b60daa8cbbc2e116d760b4e222e6a558d42ff04f
29s Normal Ready artifactgenerator/my-app ExternalArtifact/default/my-app-composite2 reconciled with revision latest@sha256:8d2c4cd4c64a2ba0512b3367660a6663e5cebdfe17204a7bf2d91cb571f8a92a
29s Normal Ready artifactgenerator/my-app external artifacts reconciled: default/my-app-composite (sha256:21f6a626fe6ad23fc6fb1c02b60daa8cbbc2e116d760b4e222e6a558d42ff04f), default/my-app-composite2 (sha256:8d2c4cd4c64a2ba0512b3367660a6663e5cebdfe17204a7bf2d91cb571f8a92a)There was a problem hiding this comment.
I think if we join the msgs with \n instead of , it will be fine 👍 @stefanprodan
There was a problem hiding this comment.
Cool, updated code and description with latest changes.
|
Please run |
184db32 to
d01dbfd
Compare
Signed-off-by: Renato Vassão <renato.vassao@mindbodyonline.com>
d01dbfd to
81fa84f
Compare
stefanprodan
left a comment
There was a problem hiding this comment.
LGTM
Thanks @renatovassaomb 🏅
Summary
Adding GitOps Toolkit EventRecorder to ArtifactGenerator Reconciler.
This will make generated events to be forwarded to the specified event recorder webhook, which usually is the notifications controller.
Fixes #307.
Changes
ArtifactGeneratorReconciler.Testing
kind create cluster docker build -t fluxcd/source-watcher:latest . kind load docker-image fluxcd/source-watcher:latestflux install --components="notification-controller"--events-addrflag in source-watcher Deployment and deploy it