Fix missing gitlabUserUsername definition for MR and Note triggers

[ljackiewicz]

It seems that gitlabUserUsername variable is not set in case of build triggered by MergeRequest event and Note (comments) event.

Additionally, gitlabUserName and gitlabUserEmail variables for these triggers (MR and Note) was changed to their real values from GitLab webhooks. In case of MR Event user is an author of specific MR, and in case of Note (Comments) Event user is an author of specific comment in MR, not author of last commit.

The proposed changes have been checked on a local GitLab instance (launched from src/docker).

[ljackiewicz]

It seems that the jenkins build didn't run for the last changes added, is there any way to trigger it manually?

[krisstern]

Thanks @ljackiewicz! Let me do a review of your PR now. Will get back to you within a few hours.

[krisstern]

Looks okay so far... @ljackiewicz If it looks okay to you too I will merge it as is.

[krisstern]

Actually this only checks for when the User's name contains "Administrator" and the User's username is "root" by design. But there should be no harm done by adding this code snippet.

[krisstern]

I think these are the tests we need for this PR I guess, which I have added/updated.

[ljackiewicz]

Thanks for the code review and your comments, but unfortunately, it's not look okay to me, because now this PR does not fix described problem and I'm not quiet sure why the proposed changes were overwritten.

I tried to explain it above, but though the User object has a username field defined and we can call the getUsername method on it, this value is not set for the author of the commit, the GitLab webhook passes only the name and email (if the email is set to public for the GitLab account) values.

Example request for MR Event: https://github.com/jenkinsci/gitlab-plugin/blob/master/src/test/resources/com/dabsquared/gitlabjenkins/webhook/build/MergeRequestEvent.json#L63

Example request for Note Event: https://github.com/jenkinsci/gitlab-plugin/blob/master/src/test/resources/com/dabsquared/gitlabjenkins/webhook/build/NoteEvent.json#L102

So the hook.getObjectAttributes().getLastCommit().getAuthor().getUsername() call for MergeRequestTriggerHandlerImpl and the hook.getMergeRequest().getLastCommit().getAuthor().getUsername() call for NoteTriggerHandlerImpl will return Null value, so the gitlabUserUsername variable still won't be set for these triggers.

[krisstern]

If the UserUsername cannot be returned here with hook.getObjectAttributes().getLastCommit().getAuthor().getUsername() then we will need some upstream changes. I don't think we are supposed to stray from the pattern prescribed, as the suggested hook.getUser().getUsername() would obviously return something not related to the Last Commit Author.

[ljackiewicz]

Yeah, and I mentioned that it can be confusing that gitlabUserName and gitlabUserUsername can represent two different persons.
It was probably a mistake that the author of the last commit was set as the author of the merge request (in case of MR Event) or comment (in case of Note Event).

[krisstern]

Then if we were to follow your logic the test cases below will need to be changed as well to include the username:

• gitlab-plugin/src/test/resources/com/dabsquared/gitlabjenkins/webhook/build/MergeRequestEvent.json

  Line 63 in bcaa2cf

  "author": {

• gitlab-plugin/src/test/resources/com/dabsquared/gitlabjenkins/webhook/build/NoteEvent.json

  Line 102 in bcaa2cf

  "author": {

[ljackiewicz]

It's not a mistake in existing test cases, these represent very well the real ones. I don't know why, but the GitLab webhook will never pass the username value of an author of the commit by design.

[krisstern]

It's not a mistake in existing test cases, these represent very well the real ones. I don't know why, but the GitLab webhook will never pass the username value of an author of the commit by design.

It is exactly my point. I don't mean there are mistakes in the existing test cases, but if your code works we will need to update as well as pass the new tests as well. Maybe we could work together to figure something out.

[ljackiewicz]

I misunderstand you then, and maybe I'm a bit stubborn, but I would like to ask do you completely rejecting an idea to make some breaking changes to the project and add gitlabLastCommiterName and gitlabLastCommiterEmail variables mentioned earlier and change the values of the gitlabUserName and gitlabUserEmail variables?

[krisstern]

I misunderstand you then, and maybe I'm a bit stubborn, but I would like to ask do you completely rejecting an idea to make some breaking changes to the project and add gitlabLastCommiterName and gitlabLastCommiterEmail variables mentioned earlier and change the values of the gitlabUserName and gitlabUserEmail variables?

Hi @ljackiewicz Actually I think it would be good to have the both the gitlab last committer name and gitlab username as variables. I am not sure if this would be breaking though (it probably is) but will need to see how other parts of the codebase is impacted with this change.

[krisstern]

Any updates? @ljackiewicz

[ljackiewicz]

@krisstern Unfortunately, I don't currently have enough time to make these changes, but I think I will try to do it as soon as possible.

[zeteref]

Hi, i have encountered a small bug related to this change. If a MR is created from tag/commitid instead of a branch hook.getObjectAttributes().getLastCommit().getAuthor().getUsername() raises nullpointerexception.

[ljackiewicz]

@zeteref Can you provide some more information about the problem you encountered, and are you sure that you describe it well? Because there are nowhere in code call of hook.getObjectAttributes().getLastCommit().getAuthor().getUsername(). It was just krisstern's suggestion, but since this call will always return null, it should never be used.

I understand that you manually built and installed the plugin based on changes provided in this PR?