Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[RFC] Flux-CDEvent Provider #4828
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
base: main
Are you sure you want to change the base?
[RFC] Flux-CDEvent Provider #4828
Changes from 2 commits
f8d20e5
4ccde7b
8081448
42b45b1
5de6bed
bb8448b
8678dec
1a5d94b
987b99c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It would be easier to comment on the individual lines if the paragraph is wrapped, maybe at 80 or 120 characters, whatever you prefer.
"event source" may fit well here, instead of "source event", indicating that the event source, the controllers, are responsible for meaningful data.
I don't think this needs to be mentioned in proposal. The corresponding github issues can have such implementation details.
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 would like to discuss this further about adding a new
mapping
spec field for CDEvents specifically.My initial understanding before this proposal was that the working of flux components are well defined which could be mapped to CDEvent event types in a deterministic way. Specific events from source-controller can be a kind of Source Code Control Event, events from kustomize-controller and helm-controller can be a kind of Continuous Deployment Event, etc.
I think the flux events can be categorized accordingly and statically configured to specific CDEvents event types. Any new event introduced in flux should also be categorized as a specific CDEvent. In notification-controller, the event handler can parse the events and forward them to message broker as per the fixed event categories.
I may be unaware of certain use cases of CDEvents that require such custom mapping, as I haven't used it myself.
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.
Hi sunny, apologies for the slow response, we have decided to in fact switch to an approach that focuses more on a mapping that is defined in the way you described rather than the custom mapping we had considered before. I will make the changes to the RFC proposal to reflect this.
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 the latest HelmRelease v2 API, I can't find these being used. We had these in the old API.