RFC : CDEvents Webhook Adapter design review

[rjalander]

Creating this PR, to review the current design approach and propose any changes that might help in creating a common cdevents-translator library.

Adding a page for Gerrit-CDEvents integration and mapping for various Gerrit events to CDEvents.

[afrittoli]

How this will work in terms of supporting multiple types?
Will the library provide an interface that various implementors can implement independently?

[rjalander]

yes an interface EventTranslator will be created with the implementation needed for TranslateEvent() for different source of events(GitHub/Gerrit`/...etc)

type EventTranslator interface {
	TranslateEvent()
}

And will have a common methods to create and send CDEvents, will update the design as per this.

[afrittoli]

The following just for info.

Tekton Triggers provides and API server that can reply to webhooks. It supports incoming CloudEvents as well as generic webhooks and it provides a powerful templating mechanism to extract information from incoming payloads and produce a YAML out of them. I used it in this POC https://github.com/afrittoli/cdeventer.

The approach of CDEventer is a bit more complex, but it has the advantage of decoupling the processing of incoming messages, which is then implemented as a set of input specific TriggerBindings, from the actual sending of the target CDEvents, through the use of an intermediate YAML.

The downside of CDEventer is the extra dependency to Tekton (and thus K8s).

[rjalander]

Thank you @xibz for directing various approaches for the translator.

@afrittoli @xibz please let us know your views on these design proposals and review.

[xibz]

This looks much better @rjalander! One thing I noticed missing was a final "Conclusions" to summarize which was preferred by you, etc. Or did you want to leave that up for discussion?

[xibz]

I'd probably rename this to "Using Go plugins to implement different Translators"

[rjalander]

updated

[xibz]

"Using RPC to add new Translators"

[rjalander]

updated

[xibz]

nit: // [.]+

[xibz]

Suggested change

	//Fields for Gerrit event Translator
	// Fields for Gerrit event Translator

[xibz]

plugin system in Go. It is a

[xibz]

I would remove the initial "But"

[rjalander]

This looks much better @rjalander! One thing I noticed missing was a final "Conclusions" to summarize which was preferred by you, etc. Or did you want to leave that up for discussion?

Thank you @xibz for reviewing this PR,

Yes I think we can discuss in CDEvents Working Group meeting and reach a conclusion to select the most suitable one

[e-backmark-ericsson]

Some general comments on this RFC:

• A basic principle in CDEvents should be that the events are sent by their source itself, or as close to that source as possible. I understand that it will be challenging to get all tools (SCM tools in this case mostly) to adapt sending CDEvents, so we might need something to fill that gap. At least shor/mid-term. It would be good to see some statement about that in the RFC.
• The RFC seems to propose a one-directional translation only, from XYZ to CDEvents. Are there ideas on translating events the other way around as well? I believe it would be a valid limitation to only support one-way translation, and thus that can be clearly stated in the introduction to the RFC. Translating from CDEvents to XYZ could require a much more complex set up, depending on what downstream systems/tools to support.
• Could/should we limit what interface protocols this translator would support towards the SCM (or other) systems? Most (all?) systems support webhooks, so maybe we should limit to that? Gerrit also supports their own 'stream-events' over ssh (and also Kafka through a plugin), but I don't think we need to support that as the webhooks support is there..
• I'd like to see a clearer scope of the intended solution. 99% of it talks about SCM related events. Could/should we limit this service to only manage SCM events, or should it be kept open for other types of CDEvents as well?
• Based on my reflections above, I'm not sure about the term 'cdevents-translator' and 'translations' here. I believe the service should have a name that reflect better the intended scope of it. Some suggestions: 'scm_event_producer', 'scm_adapter', 'any_to_cdevents_adapter', 'webhook_cdevents_producer'
• It will be easier to grasp the proposal if a simple picture was included, showing where services are deployed and their interfaces to the SCM system(s) and event brokers etc.
• A consumer of these events will eventually want to provide feedback on the source change in the SCM system, so it would be great to see that included in use cases and in a schematic picture of this proposal. I don't expect this service itself to manage that feedback, or is that considered as an option as well?

Bottom line: I propose that this initiative is limited to manage SCM related events only, and that it is one-directional. The name should preferably in some way reflect that.

[rjalander]

Some general comments on this RFC:

• A basic principle in CDEvents should be that the events are sent by their source itself, or as close to that source as possible. I understand that it will be challenging to get all tools (SCM tools in this case mostly) to adapt sending CDEvents, so we might need something to fill that gap. At least shor/mid-term. It would be good to see some statement about that in the RFC.
• The RFC seems to propose a one-directional translation only, from XYZ to CDEvents. Are there ideas on translating events the other way around as well? I believe it would be a valid limitation to only support one-way translation, and thus that can be clearly stated in the introduction to the RFC. Translating from CDEvents to XYZ could require a much more complex set up, depending on what downstream systems/tools to support.
• Could/should we limit what interface protocols this translator would support towards the SCM (or other) systems? Most (all?) systems support webhooks, so maybe we should limit to that? Gerrit also supports their own 'stream-events' over ssh (and also Kafka through a plugin), but I don't think we need to support that as the webhooks support is there..
• I'd like to see a clearer scope of the intended solution. 99% of it talks about SCM related events. Could/should we limit this service to only manage SCM events, or should it be kept open for other types of CDEvents as well?
• Based on my reflections above, I'm not sure about the term 'cdevents-translator' and 'translations' here. I believe the service should have a name that reflect better the intended scope of it. Some suggestions: 'scm_event_producer', 'scm_adapter', 'any_to_cdevents_adapter', 'webhook_cdevents_producer'
• It will be easier to grasp the proposal if a simple picture was included, showing where services are deployed and their interfaces to the SCM system(s) and event brokers etc.
• A consumer of these events will eventually want to provide feedback on the source change in the SCM system, so it would be great to see that included in use cases and in a schematic picture of this proposal. I don't expect this service itself to manage that feedback, or is that considered as an option as well?

Bottom line: I propose that this initiative is limited to manage SCM related events only, and that it is one-directional. The name should preferably in some way reflect that.

@e-backmark-ericsson Thank you very much for your comments,

• Updated the design including the first 4 comments as part of Goals, Non-Goals and Motivation and Rationale
• And about the name change we discussed in the CDEvents Workgroup meeting yesterday, is that fine to change as webhook_cdevents_translator or webhook_cdevents_adapter, please comment.
• Will try to add a picture for this proposal as suggested
• I think a feedback from the consumer will be provided back to the message-broker with an another CDEvent, but the event will not be translated/send to SCM system, as stated above this translator supports one-way translation.

[e-backmark-ericsson]

To me it makes sense to rename this proposed implementation to webhook_cdevents_adapter. A translator to me implies that there would be a one-to-one translation, but that might not always be the case I think.

[rjalander]

@e-backmark-ericsson @afrittoli @xibz Thank you all for reviewing and providing your comments.
Please approve this RFC If no other comments.

[e-backmark-ericsson]

I don't have a valid opinion on whether alternative 3 or any of the other are preferred but I'd say go ahead to this and if needed we could always make a different attempt later.
The proposal has some inconsistencies in the naming between 'adapter' and 'translator', but we could sort those out during the actual PR review later. I assume that based on this RFC we will create a repo under the cdevents org called 'webhook_cdevents_adapter'.

[rjalander]

Yes I requested @afrittoli to create project under cdevents org with the name "webhook-cdevents-adapter" (replaced _ with -), just to follow other projects naming conventions under cdevents org

And about using translator, referring plugin implementation projects for various SCM tools as <scm_tool_name>-translator-cdevents(can be a separate project under different org.)

Ex: gerrit-translator-cdevents and the interface name EventTranslator.TranslateEvent() to be implemented.

Please suggest, If I can rename this too with gerrit-adaptor-cdevents and the interface name EventAdaptor.TranslateEvent() to implement ?

[rjalander]

Yes I requested @afrittoli to create project under cdevents org with the name "webhook-cdevents-adapter" (replaced _ with -), just to follow other projects naming conventions under cdevents org

And about using translator, referring plugin implementation projects for various SCM tools as <scm_tool_name>-translator-cdevents(can be a separate project under different org.)

Ex: gerrit-translator-cdevents and the interface name EventTranslator.TranslateEvent() to be implemented.

Please suggest, If I can rename this too with gerrit-adaptor-cdevents and the interface name EventAdaptor.TranslateEvent() to implement ?

As discussed in one of the SIG event meets, keeping the plugins name with translator word and using adapter for CDEvents webhook.

[rjalander]

@afrittoli @xibz can you please re-review and approve If no other comments with this RFC.

[afrittoli]

Thanks @rjalander - lgtm!
Commits need to squashed - we can do a squash and merge too.

[rjalander]

Can we merge this PR If no other comments. Thank you.