-
Notifications
You must be signed in to change notification settings - Fork 599
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
base: main
Are you sure you want to change the base?
Conversation
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 |
6d5ee27
to
14079bc
Compare
a2c7092
to
f9571f8
Compare
7b62750
to
0928016
Compare
51936fa
to
ad059ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @matheuscscp 🏅
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` |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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.
Signed-off-by: Matheus Pimenta <[email protected]>
ad059ae
to
e338913
Compare
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the image tag is not yet accessible in event annotation yet?
|
||
#### Story 1 | ||
|
||
> As a user, I want to embed FluxCD into my GitHub Workflow in a way that it only succeeds if |
There was a problem hiding this comment.
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.
No description provided.