gerrit-translator-cdevents : Plugin implementation to translate branch and repository events #1

rjalander · 2024-03-19T16:28:50Z

Implementation of EventTranslator interface that is exposed by CDEvents Webhook Adapter to translate gerrit events into CDEvents.
This PR includes the changes to translate below Project and Branch related gerrit events into CDEvents, with the mappings as below

CDEvent Type	Gerrit Event Type
dev.cdevents.repository.created	project-created
dev.cdevents.repository.modified	project-head-updated
dev.cdevents.branch.created	ref-updated
dev.cdevents.branch.deleted	ref-updated

Note: Implementation as per the design RFC: gerrit-translator-cdevents-plugin and mapping events gerrit-cdevents

Signed-off-by: Jalander Ramagiri @rjalander

Signed-off-by: Jalander Ramagiri <[email protected]>

xibz

@rjalander This looks like a good start. I have some questions and some ideas on how to make this a little more scalable.

README.md

xibz · 2024-04-11T20:11:12Z

pkg/gerrit/create_cdevents.go

+	cdEvent, err := sdk.NewRepositoryCreatedEvent()
+	if err != nil {
+		log.Printf("Error creating CDEvent RepositoryCreatedEvent %s\n", err)
+		return "", err


One thing that might be appropriate to do is wrap these in our own Error. I think a little digging is required here, but what do the errors look like when returned from the SDK?

For instance, if the error that is returned is something like error: pipe broken than that's not too useful. Wrapping these errors with something that provides more context may be necessary

Right now we have two repos:

The plugin webhook adapter thingy and translator(s).

The webhook adapter can provide a set of Error types that can be wrapped, and I think that would at least give some guidance on how the adapter should handle such errors, e.g. should we return a proper status code based on error type for example.

This will require some design, but I think it's an important call out and consideration.

Will create an issue for handling these error scenarios and discuss design considerations here.

@rjalander could you share a link to the issue?

cdevents/webhook-adapter#5

pkg/gerrit/create_cdevents.go

xibz · 2024-04-11T20:14:46Z

pkg/gerrit/create_cdevents.go

+	return cdEventStr, nil
+}
+
+func (projectHeadUpdated *ProjectHeadUpdated) RepositoryModifiedCDEvent() (string, error) {


Hmm, this seems a little odd, and I was thinking of a much different approach. Can you schedule a meeting with me? I want to go over some design consideration here

Signed-off-by: Jalander Ramagiri <[email protected]>

xibz

Had just two comments, but I think we still need to go over the implementation of the go tags. It'll make this much easier in the future. Can you schedule something for this week?

.github/workflows/ci.yml

pkg/gerrit/logger.go

wrong click

xibz

requesting some changes

Signed-off-by: Jalander Ramagiri <[email protected]>

xibz

Just adding a comment about our prior meeting with Jalander and Andrew.

The goal is to utilize copygen or some library that supports easy copying of structs without needing to manually write them ourselves.

There was some concerns around perfection and feature creep, so we decided that if we are unable to get struct tags in the go sdk by mid July, we will go ahead with the duplicated copying logic until struct tags exist.

rjalander · 2024-06-12T14:27:52Z

Just adding a comment about our prior meeting with Jalander and Andrew.

The goal is to utilize copygen or some library that supports easy copying of structs without needing to manually write them ourselves.

There was some concerns around perfection and feature creep, so we decided that if we are unable to get struct tags in the go sdk by mid July, we will go ahead with the duplicated copying logic until struct tags exist.

created this PR cdevents/sdk-go#82 to support copygen for go-sdk

xibz

Thanks for all the changes @rjalander. I will go ahead and approve this. The goal, however, is to eventually use some copier library to make it easy for translators to be developed. I will create a separate issue for that.

rjalander · 2024-07-01T13:05:39Z

Thanks for all the changes @rjalander. I will go ahead and approve this. The goal, however, is to eventually use some copier library to make it easy for translators to be developed. I will create a separate issue for that.

Thanks @xibz, We will address this issue and initiate a separate pull request to implement the copying of metadata utilizing a designated copier library.

@afrittoli, could you please review this PR and approve it if there are no comments?

README.md

afrittoli

One thing that I noticed is that the plugin here returns CDEvents as JSON.

Currently the AsCloudEvent function takes a CDEvent reader object as input, and there is no obvious way to send a JSON CDEvent as CloudEvent other than parsing it back into an object and using AsCloudEvent.

Perhaps we'll need a new method in the SDK. or a different interface here

go.mod

rjalander · 2024-07-01T16:44:43Z

One thing that I noticed is that the plugin here returns CDEvents as JSON.

Currently the AsCloudEvent function takes a CDEvent reader object as input, and there is no obvious way to send a JSON CDEvent as CloudEvent other than parsing it back into an object and using AsCloudEvent.

Perhaps we'll need a new method in the SDK. or a different interface here

I tried using an interface which accepts cdevent object as a returned value instead of cdevent JSON string in the webhook-adapter, but that has some difficulties in generating grpc client code using protoc-gen for custom datatypes.
So using JSON string as a response for this interface, https://github.com/cdevents/webhook-adapter/blob/main/pkg/proto/translator.proto#L37

And in webhook-adapter the received cdevent JSON string will be converted to cdevent object using NewFromJsonString(event string) (CDEvent, error), but looks like this method is replaced in the main go-sdk (need to use differently once webhook-adapter upgraded to v0.4 sdk)

Signed-off-by: Jalander Ramagiri <[email protected]>

afrittoli · 2024-07-02T09:10:49Z

One thing that I noticed is that the plugin here returns CDEvents as JSON.
Currently the AsCloudEvent function takes a CDEvent reader object as input, and there is no obvious way to send a JSON CDEvent as CloudEvent other than parsing it back into an object and using AsCloudEvent.
Perhaps we'll need a new method in the SDK. or a different interface here

I tried using an interface which accepts cdevent object as a returned value instead of cdevent JSON string in the webhook-adapter, but that has some difficulties in generating grpc client code using protoc-gen for custom datatypes. So using JSON string as a response for this interface, https://github.com/cdevents/webhook-adapter/blob/main/pkg/proto/translator.proto#L37

And in webhook-adapter the received cdevent JSON string will be converted to cdevent object using NewFromJsonString(event string) (CDEvent, error), but looks like this method is replaced in the main go-sdk (need to use differently once webhook-adapter upgraded to v0.4 sdk)

I could not avoid having the NewFromJsonString method being version specific - so in the new SDK you have one version for v0.3 and one for v0.4.

There is a NewFromJsonBytes method that works with golang generics and can be used across spec versions, but the version specific NewFromJsonString is more convenient to use.

rjalander · 2024-07-02T15:48:28Z

One thing that I noticed is that the plugin here returns CDEvents as JSON.
Currently the AsCloudEvent function takes a CDEvent reader object as input, and there is no obvious way to send a JSON CDEvent as CloudEvent other than parsing it back into an object and using AsCloudEvent.
Perhaps we'll need a new method in the SDK. or a different interface here

I tried using an interface which accepts cdevent object as a returned value instead of cdevent JSON string in the webhook-adapter, but that has some difficulties in generating grpc client code using protoc-gen for custom datatypes. So using JSON string as a response for this interface, https://github.com/cdevents/webhook-adapter/blob/main/pkg/proto/translator.proto#L37
And in webhook-adapter the received cdevent JSON string will be converted to cdevent object using NewFromJsonString(event string) (CDEvent, error), but looks like this method is replaced in the main go-sdk (need to use differently once webhook-adapter upgraded to v0.4 sdk)

I could not avoid having the NewFromJsonString method being version specific - so in the new SDK you have one version for v0.3 and one for v0.4.

There is a NewFromJsonBytes method that works with golang generics and can be used across spec versions, but the version specific NewFromJsonString is more convenient to use.

Ok, will use the same function NewFromJsonString after webhook-adaper is upgraded to v0.4

rjalander · 2024-07-03T10:16:53Z

@afrittoli, could you please approve it if there are no other comments?

Initial changes to translate branch and repository events

3626fe7

Signed-off-by: Jalander Ramagiri <[email protected]>

rjalander requested review from xibz, afrittoli and adamkenihan March 20, 2024 09:35

adamkenihan approved these changes Mar 20, 2024

View reviewed changes

rjalander requested review from adamkenihan and removed request for adamkenihan March 20, 2024 15:51

Adding workflow CI to test lint and unit test

0eb8e03

Signed-off-by: Jalander Ramagiri <[email protected]>

xibz requested changes Apr 11, 2024

View reviewed changes

addressing review comments to add Makefile and zap logger

894350a

Signed-off-by: Jalander Ramagiri <[email protected]>

rjalander requested a review from xibz April 26, 2024 12:34

Fixing lint issue in logger file

b8eae45

Signed-off-by: Jalander Ramagiri <[email protected]>

xibz previously approved these changes May 1, 2024

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

pkg/gerrit/logger.go Outdated Show resolved Hide resolved

xibz requested changes May 1, 2024

View reviewed changes

update build flags and logger method to init

d4e78ea

Signed-off-by: Jalander Ramagiri <[email protected]>

xibz reviewed Jun 7, 2024

View reviewed changes

xibz approved these changes Jun 29, 2024

View reviewed changes

afrittoli reviewed Jul 1, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

afrittoli reviewed Jul 1, 2024

View reviewed changes

go.mod Show resolved Hide resolved

updating README text corrected

d3571c4

Signed-off-by: Jalander Ramagiri <[email protected]>

afrittoli approved these changes Jul 3, 2024

View reviewed changes

rjalander mentioned this pull request Jul 3, 2024

Improve error handling for webhook adapter cdevents/webhook-adapter#5

Open

adamkenihan approved these changes Jul 3, 2024

View reviewed changes

afrittoli merged commit 5b5569b into cdevents:main Jul 3, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gerrit-translator-cdevents : Plugin implementation to translate branch and repository events #1

gerrit-translator-cdevents : Plugin implementation to translate branch and repository events #1

rjalander commented Mar 19, 2024 •

edited

Loading

xibz left a comment

xibz Apr 11, 2024

rjalander Apr 22, 2024

afrittoli Jul 3, 2024

rjalander Jul 3, 2024

xibz Apr 11, 2024

xibz left a comment

xibz left a comment

xibz left a comment

rjalander commented Jun 12, 2024

xibz left a comment

rjalander commented Jul 1, 2024

afrittoli left a comment

rjalander commented Jul 1, 2024

afrittoli commented Jul 2, 2024

rjalander commented Jul 2, 2024

rjalander commented Jul 3, 2024

gerrit-translator-cdevents : Plugin implementation to translate branch and repository events #1

gerrit-translator-cdevents : Plugin implementation to translate branch and repository events #1

Conversation

rjalander commented Mar 19, 2024 • edited Loading

xibz left a comment

Choose a reason for hiding this comment

xibz Apr 11, 2024

Choose a reason for hiding this comment

rjalander Apr 22, 2024

Choose a reason for hiding this comment

afrittoli Jul 3, 2024

Choose a reason for hiding this comment

rjalander Jul 3, 2024

Choose a reason for hiding this comment

xibz Apr 11, 2024

Choose a reason for hiding this comment

xibz left a comment

Choose a reason for hiding this comment

xibz left a comment

Choose a reason for hiding this comment

xibz left a comment

Choose a reason for hiding this comment

rjalander commented Jun 12, 2024

xibz left a comment

Choose a reason for hiding this comment

rjalander commented Jul 1, 2024

afrittoli left a comment

Choose a reason for hiding this comment

rjalander commented Jul 1, 2024

afrittoli commented Jul 2, 2024

rjalander commented Jul 2, 2024

rjalander commented Jul 3, 2024

rjalander commented Mar 19, 2024 •

edited

Loading