Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Custom Event Metadata from Annotations #4809

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

matheuscscp
Copy link
Contributor

No description provided.

@moritzschmitz-oviva
Copy link

This is great! Thanks again for picking this up, @matheuscscp 🎉 !

I have nothing to add. It explains the use case very well.

Regarding the API group, I don't know enough about the benefits of implementing the EventRecorder interface. Otherwise, your first suggestion looks sufficient.

@matheuscscp matheuscscp force-pushed the rfc-7-custom-event-metadata branch 2 times, most recently from 6d5ee27 to 14079bc Compare June 10, 2024 11:36
@stefanprodan stefanprodan changed the title Add RFC 0007 - Custom Event Metadata from Annotations [RFC] Custom Event Metadata from Annotations Jun 13, 2024
@matheuscscp matheuscscp force-pushed the rfc-7-custom-event-metadata branch 4 times, most recently from 7b62750 to 0928016 Compare July 4, 2024 15:44
@matheuscscp matheuscscp force-pushed the rfc-7-custom-event-metadata branch 2 times, most recently from 51936fa to ad059ae Compare July 4, 2024 15:48
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @matheuscscp 🏅

Comment on lines +151 to +161
couple of alternatives:

* `notification.toolkit.fluxcd.io`. This alternative emphasizes the close relationship of the
feature with notification-controller and does not introduce a new Flux API group.
* `event.toolkit.fluxcd.io`. This alternative decouples the feature from notification-controller
and brings the concept closer to Kubernetes Events. In fact, our implementation builds on the
`EventRecorder` interface from the Go package `k8s.io/client-go/tools/record`, hijacking the
`AnnotatedEventf()` logic to send events not only to Kubernetes but also notification-controller.
This would, however, introduce a new Flux API Group.

Chosen alternative: `event.toolkit.fluxcd.io`
Copy link
Contributor

@darkowlzz darkowlzz Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should summarize this instead of listing the possible options. Just stating the finalized API group and mentioning about the EventRecorder interface should be good enough.
If this, now finalized, API group is mentioned from the beginning of the RFC, in the summary and proposal, the alternative notification.toolkit.fluxcd.io could be included in the alternatives section. If we do that, we don't need to introduce the API group in design details section and instead focus on elaborating how we'll add it using the EventRecorder interface.

Flux users often run into situations where they wish to send custom, static metadata fields defined
in Flux objects on the events dispatched by the respective controller to notification-controller.
This proposal offers a solution for supporting those use cases uniformly across all Flux controllers
by sending annotation keys that are prefixed with a well-defined API Group.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The phrase "sending annotation keys" got my attention here and it feels a little confusing even after considering the rest of the RFC as we don't get into more details to highlight the importance of the metadata prefix which are specific to how the notification-controller event server interprets the received events. This is also phrased similarly in the proposal section. I think what's sent between controllers is an implementation detail that can be elaborated briefly in the design details section. In summary and proposal, I believe we are proposing a solution from a Flux user's perspective and from their perspective, they set annotations with a specific prefix. It may be better to rephrase accordingly and mention about the events, their annotation keys and how it'll be received by the notification-controller event server in the design details.

name: podinfo
```

Should cause notification-controller to propagate an event like the one below (most fields omitted for brevity):
Copy link
Contributor

@darkowlzz darkowlzz Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case it's not obvious for readers who are not familiar with image-automation-controller, it may help to mention briefly above that IAC will update the chart version and the annotation value with the same latest values, which will be propagated by the notification controller. Just slight details.

Comment on lines +96 to +111
apiVersion: helm.toolkit.fluxcd.io/v2
kind: HelmRelease
metadata:
name: podinfo
namespace: flux-system
annotations:
event.toolkit.fluxcd.io/image: ghcr.io/stefanprodan/podinfo # {"$imagepolicy": "flux-system:podinfo:name"}
event.toolkit.fluxcd.io/imageTag: 6.5.0 # {"$imagepolicy": "flux-system:podinfo:tag"}
spec:
chart:
spec:
chart: podinfo
version: 6.5.0 # {"$imagepolicy": "flux-system:podinfo:tag"}
sourceRef:
kind: HelmRepository
name: podinfo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
apiVersion: helm.toolkit.fluxcd.io/v2
kind: HelmRelease
metadata:
name: podinfo
namespace: flux-system
annotations:
event.toolkit.fluxcd.io/image: ghcr.io/stefanprodan/podinfo # {"$imagepolicy": "flux-system:podinfo:name"}
event.toolkit.fluxcd.io/imageTag: 6.5.0 # {"$imagepolicy": "flux-system:podinfo:tag"}
spec:
chart:
spec:
chart: podinfo
version: 6.5.0 # {"$imagepolicy": "flux-system:podinfo:tag"}
sourceRef:
kind: HelmRepository
name: podinfo
apiVersion: helm.toolkit.fluxcd.io/v2
kind: HelmRelease
metadata:
name: podinfo
namespace: flux-system
annotations:
event.toolkit.fluxcd.io/image: latest # {"$imagepolicy": "flux-system:podinfo"}
spec:
chart:
spec:
chart: podinfo
sourceRef:
kind: HelmRepository
name: podinfo
values:
image:
tag: latest # {"$imagepolicy": "flux-system:podinfo:tag"}


#### Story 1

> As a user, I want to embed FluxCD into my GitHub Workflow in a way that it only succeeds if
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace FluxCD with Flux everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants