sonyxperiadev / gerrit-events

MIT License
47 stars 62 forks source link

Topic Changed Event does not propagate patch set #119

Open danielbebbernovacode opened 2 years ago

danielbebbernovacode commented 2 years ago

Hello,

When a topic changed event is triggered the event does not contain a patchset:

Aug 09 15:29:29 nc jenkins[886]: 2022-08-09 13:29:29.872+0000 [id=332]        INFO        c.s.h.p.g.t.g.ToGerritRunListener#onTriggered: Project [VersionBumpJenkins] triggered by Gerrit: [com.sonymobile.tools.gerrit.gerritevents.dto.events.TopicChanged@45c6eec8]
Aug 09 15:29:29 nc jenkins[886]: 2022-08-09 13:29:29.873+0000 [id=332]        INFO        c.s.h.p.g.t.h.EventListener#schedule: Project VersionBumpJenkins Build Scheduled: true By event: 542081
Aug 09 15:29:37 nc jenkins[886]: 2022-08-09 13:29:37.244+0000 [id=22014]        INFO        c.s.h.p.g.t.g.ToGerritRunListener#onStarted: Gerrit build [VersionBumpJenkins #11] Started for cause: [GerritCause: com.sonymobile.tools.gerrit.gerritevents.dto.events.TopicChanged@45c6eec8 silent: false].
Aug 09 15:29:37 nc jenkins[886]: 2022-08-09 13:29:37.244+0000 [id=22014]        INFO        c.s.h.p.g.t.g.ToGerritRunListener#onStarted: MemoryStatus:
Aug 09 15:29:37 nc jenkins[886]:   Project/Build: [VersionBumpJenkins]: [#11: null] Completed: false
Aug 09 15:29:38 nc jenkins[886]: 2022-08-09 13:29:38.640+0000 [id=19668]        SEVERE        c.s.t.g.g.w.c.AbstractSendCommandJob#sendCommand: Could not run command gerrit review 542081,<PATCHSET> --message 'Build Started http://192.168.178.22:8080/job/VersionBumpJenkins/11/ ' --verified 0 --code-review 0 --tag autogenerated:jenkins-gerrit-trigger
Aug 09 15:29:38 nc jenkins[886]: com.sonymobile.tools.gerrit.gerritevents.ssh.SshException: fatal: "542081,<PATCHSET>" is not a valid patch set (1)

This causes the our jenkins to being unable to send messages to gerrit changes where the topic was changed.

rsandell commented 1 year ago

That looks like a bug in the Jenkins Gerrit Trigger plugin, not this library? Or does the topic-changed event contain the patchset number but it isn't read correctly by this library?

ckreisl commented 1 year ago

@rsandell the log is from the Gerrit Trigger Plugin. Quick question since I saw this during debugging: https://github.com/sonyxperiadev/gerrit-events/blob/master/src/main/java/com/sonymobile/tools/gerrit/gerritevents/dto/events/TopicChanged.java#L125

    @Override
    public boolean isScorable() {
        return false;
    }

Why is topic-changed not a scorable event? Or generally spoken when is an event "scorable"?

e.g. the topic-changed event would trigger the build but if I can't score back to Gerrit there are only comments send back to the Gerrit patchset. So the issue right now is simply that the "patchset / currentPatchset" for this Gerrit Topic event is just "null" and can't be resolved in the ParameterExpander.class in the Gerrit Trigger Plugin. Which is shown above in the log and thus results in an invalid Gerrit query string.

From the Gerrit Docs https://gerrit-review.googlesource.com/Documentation/cmd-stream-events.html they metion:

SCHEMA
The JSON messages consist of nested objects referencing the change, patchSet, account involved, and other attributes as appropriate. 
Note that any field may be missing in the JSON messages, so consumers of this JSON stream should deal with that appropriately.

But what I get with a test is the following (Gerrit vers. 3.7.0.):

So there is no patchSet reference which means we have to fetch the information based on the change.number for the whole patchset.

{
    "changer": {
        "name": "dev",
        "email": "dev@dev.com",
        "username": "dev"
    },
    "oldTopic": "",
    "change": {
        "project": "test-a",
        "branch": "main",
        "topic": "debug",
        "id": "Ic66c7bf7e6f5ae23ec137c821c5470cd11fcd6a6",
        "number": 121,
        "subject": "lol",
        "owner": {
            "name": "dev",
            "email": "dev@dev.com",
            "username": "dev"
        },
        "url": "http://127.0.0.1:5080/c/test-a/+/121",
        "commitMessage": "lol\n\nChange-Id: Ic66c7bf7e6f5ae23ec137c821c5470cd11fcd6a6\n",
        "createdOn": 1675891235,
        "status": "NEW"
    },
    "project": "test-a",
    "refName": "refs/heads/main",
    "changeKey": {
        "id": "Ic66c7bf7e6f5ae23ec137c821c5470cd11fcd6a6"
    },
    "type": "topic-changed",
    "eventCreatedOn": 1687193478
}
rsandell commented 1 year ago

An event is not "scorable" when it is no longer possible to "vote" on the change, e.g. set Verified +1. For example when a change is merged it sends out a change merged event that Jenkins can trigger on, but the result of the build can't be scored so Jenkins will then only add a comment.

Why this event was deemed not scorable we would have to ask the original author @dpursehouse and hope he can remember that far back :)

ckreisl commented 1 year ago

An event is not "scorable" when it is no longer possible to "vote" on the change, e.g. set Verified +1. For example when a change is merged it sends out a change merged event that Jenkins can trigger on, but the result of the build can't be scored so Jenkins will then only add a comment.

Why this event was deemed not scorable we would have to ask the original author @dpursehouse and hope he can remember that far back :)

ok thanks for the quick answer, then my assumption was correct and from my understanding the isScorable() function should return true; for the topic-changed event. So far this event can only happen if you change the topic in a Gerrit Patchset change. From my point of view this might require some additional options in the Gerrit Trigger Plugin if a build is actually required e.g. if the change status is abandoned or even merged.

dpursehouse commented 1 year ago

I guess I implemented it like that because a topic change doesn't cause any approvals to be removed and thus it doesn't need to be re-scored. Or maybe I just copy and pasted the code from another event. Sorry, it was a long time ago and I can't remember exactly.