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

Adding cdevents tag name for copying struct fields #82

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

Conversation

rjalander
Copy link

@rjalander rjalander commented Jun 10, 2024

This PR includes the changes to add cdevents tag name for CDEvents Subject and Context fields, this will help in copying the struct fields directly from any other source event to CDEvents type event, even the field names are different by using libraries like copygen.

//Source Event
type ProjectCreatedEvent struct {
	ProjectName string `cdevents:"name"`
	RepoURL string `cdevents:"url"`
}

//Destination Event
type RepositoryCreatedCDEvent struct {
	Name string `cdevents:"name"`
	Url string `cdevents:"url"`
}

Note:

  1. With this change If anyone using cdevents tag names and in future any of the field name changed, it will be a breaking change for the current users.(They need to update the tag name as per the change)

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

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.94%. Comparing base (f8b8b91) to head (b82880a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #82   +/-   ##
=======================================
  Coverage   79.94%   79.94%           
=======================================
  Files          48       48           
  Lines        3291     3291           
=======================================
  Hits         2631     2631           
  Misses        552      552           
  Partials      108      108           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Thanks for this @rjalander - I'm a bit at a loss about how this will beneficial, since the mapping between objects and json is already in the json tags. Do you have an example of how this would be used?

Also, it would be nice if to update the code, you can run make generate for that.

@@ -75,7 +75,7 @@ type Context struct {
// purpose of the source is to provide global uniqueness for source + id.
// The source MAY identify a single producer or a group of producer that
// belong to the same application.
Source string `json:"source" jsonschema:"required,minLength=1" validate:"uri-reference"`
Source string `json:"source" jsonschema:"required,minLength=1" validate:"uri-reference" cdevents:"context_source"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there some implied naming convention to map a field name and position in the json structure to a string?
So that context.source becomes context_source?

Copy link
Author

Choose a reason for hiding this comment

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

no, but just to differentiate subject's source with the context's source. Keeping tag name as context_source here and for subject's source using it as subject_source

@@ -41,7 +41,7 @@ var (

type {{.Subject}}{{.Predicate}}SubjectContent struct{
{{ range $i, $field := .Contents }}
{{ .Name }} {{ .Type }} `json:"{{ .NameLower }}{{ if not .Required }},omitempty{{ end }}"{{ if eq .Name "ArtifactId" }} validate:"purl"{{ end }}`
{{ .Name }} {{ .Type }} `json:"{{ .NameLower }}{{ if not .Required }},omitempty{{ end }}"{{ if eq .Name "ArtifactId" }} validate:"purl"{{ end }} cdevents:"{{ .NameLower }}"`
Copy link
Contributor

Choose a reason for hiding this comment

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

The info in the cdevents tag is already in the json tag - I'm not sure I understand the value of adding them to a new tag. Also, this tag here has a different name structure compared to those in the context.

Copy link
Author

Choose a reason for hiding this comment

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

yes, we tried using json tag info for copygen, but the same json tag will also be used for serialize/deserialize and this will create a problem in matching the field names for different sources,

For Example the source ProjectCreated Gerrit event,

type ProjectCreated struct {
	ProjectName string `json:"projectName" cdevents:"name"`
	HeadName    string `json:"headName" cdevents:"subject_id"`
	Type           string  `json:"type"`
	EventCreatedOn float64 `json:"eventCreatedOn"`
	RepoURL        string  `json:"repoURL,omitempty" cdevents:"context_source"`
}

here the json tag names projectName, headName are used to serialize gerrit event.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm missing some context or maybe knowledge about copygen to better understand what's going on.

I understand that json tag in types that represent Gerrit events is used to marshal/unmarshal gerrit events to JSON - that's the standard way of doing JSON marshalling/unmarshalling in go.

I guess cdevents tag is meant to tell copygen that it should fill the field of a struct with the CDEvents field specified in the tag. What I don't understand is why we need to add cdevents tags on the CDEvents SDK side.

Copy link
Author

Choose a reason for hiding this comment

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

Actually the same tag should be present in all the structs(Gerrit + CDEvents) to match fields and generate functions to copy fields from Gerrit event to CDEvent.
So we need to have a tag names on CDEvents SDK and the same tag names can be used to match different source events, which can be translated into CDEvents.

An example from copygen shows model struct copied into domain struct, https://github.com/switchupcb/copygen/tree/main/examples/tag#example-tag

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.

2 participants