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

Topic Changed Event does not propagate patch set #119

Open
danielbebbernovacode opened this issue Aug 9, 2022 · 5 comments
Open

Topic Changed Event does not propagate patch set #119

danielbebbernovacode opened this issue Aug 9, 2022 · 5 comments

Comments

@danielbebbernovacode
Copy link

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
Copy link
Collaborator

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
Copy link

ckreisl commented Jun 19, 2023

@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
Copy link
Collaborator

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
Copy link

ckreisl commented Jun 20, 2023

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
Copy link
Contributor

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.

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

No branches or pull requests

4 participants