Part2: gerrit-translator-cdevents : translate patchset and change source code events#3
Part2: gerrit-translator-cdevents : translate patchset and change source code events#3rjalander wants to merge 7 commits intocdevents:mainfrom
Conversation
Signed-off-by: Jalander Ramagiri <[email protected]>
Signed-off-by: Jalander Ramagiri <[email protected]>
Signed-off-by: Jalander Ramagiri <[email protected]>
Signed-off-by: Jalander Ramagiri <[email protected]>
Signed-off-by: Jalander Ramagiri <[email protected]>
|
@afrittoli @xibz could you please review this PR and share your comments. |
Signed-off-by: Jalander Ramagiri <[email protected]>
xibz
left a comment
xibz
left a comment
There was a problem hiding this comment.
Most of my comments are very nit. So this can be a follow up PR to make it more idiomatic to Go
pkg/gerrit/event.go
Outdated
| if err != nil { | ||
| return "", err | ||
| } | ||
| case "patchset-created": |
There was a problem hiding this comment.
These event types should probably be constants as that is more idiomatic to go even if they are only used here.
| if refUpdated.RefUpdate.OldRev == "0000000000000000000000000000000000000000" { | ||
| if strings.Contains(refUpdated.RefUpdate.RefName, "refs/changes") { | ||
| Log().Info("Ignoring handling ref-updated gerrit event as this is followed by patchset/change events: ", refUpdated) | ||
| return "", errors.New("ignoring translating ref-updated gerrit event") |
There was a problem hiding this comment.
Is this 0000000000000000000000000000000000000000 a zeroe'd SHA?
We should also make this a constant. My guess is this is sent by gerrit?
pkg/gerrit/translate_events.go
Outdated
| } | ||
| Log().Info("PatchsetCreated GerritEvent received for project : ", patchsetCreated.Project.Name) | ||
| patchsetCreated.RepoURL = pEvent.RepoURL | ||
| if patchsetCreated.PatchSet.Number == 1 { |
There was a problem hiding this comment.
I noticed we have a lot of logs on the info level. I wonder if we should put some of these, if not most, in debugging.
Also this 1 could be a constant like const PatchSetCreatedNum = 1 or something
Signed-off-by: Jalander Ramagiri <[email protected]>
afrittoli
left a comment
afrittoli
left a comment
There was a problem hiding this comment.
Thanks for this PR.
The mapping that is documented in the PR description should also go into some docs for this plugin, so that one can know which event is mapped to what without having to parse the code.
Apart from that, I believe the Info logging includes too much info, if this plugin was processing many events it would be too much, but this is something that could be tuned as a follow-up.
| } | ||
|
|
||
| func (patchsetCreated *PatchsetCreated) ChangeCreatedCDEvent() (string, error) { | ||
| Log().Info("Creating CDEvent ChangeCreatedEvent") |
There was a problem hiding this comment.
Having Info log level for each event created will spam logs without providing much value as the text is fixed. I would make this Debug and provide at least the event ID in there
| if err != nil { | ||
| return "", err | ||
| } | ||
| Log().Info("Translated patchset-created gerrit event into dev.cdevents.change.created CDEvent: ", cdEvent) |
There was a problem hiding this comment.
I think Info is too much here too.
3 participants
This PR includes the changes to translate below Patchset and Change related gerrit events into CDEvents, with the mappings as below
Note: Implementation as per the design RFC: gerrit-translator-cdevents-plugin and mapping events gerrit-cdevents
Signed-off-by: Jalander Ramagiri @rjalander