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

getCommits can return null #141

Merged
merged 2 commits into from
Dec 31, 2016
Merged

Conversation

CSchulz
Copy link
Contributor

@CSchulz CSchulz commented Dec 1, 2016

Dez 01, 2016 1:47:22 PM SCHWERWIEGEND hudson.plugins.tfs.TeamEventsEndpoint dispatch
Error while performing reaction to 'gitPush' event.
java.lang.NullPointerException
  at hudson.plugins.tfs.model.GitPushEvent.determineCommit(GitPushEvent.java:100)
  at hudson.plugins.tfs.model.GitPushEvent.decodeGitPush(GitPushEvent.java:120)
  at hudson.plugins.tfs.model.GitPushEvent.perform(GitPushEvent.java:44)
  at hudson.plugins.tfs.TeamEventsEndpoint.innerDispatch(TeamEventsEndpoint.java:163)
  at hudson.plugins.tfs.TeamEventsEndpoint.dispatch(TeamEventsEndpoint.java:133)
  at hudson.plugins.tfs.TeamEventsEndpoint.doGitPush(TeamEventsEndpoint.java:201) 

@cniweb
Copy link

cniweb commented Dec 1, 2016

This will not fix the problem.
Commits can be empty:
https://www.visualstudio.com/en-us/docs/integrate/get-started/service-hooks/events#git.push
@davidstaheli But why?

Chris

@davidstaheli
Copy link
Contributor

@cniweb Unfortunately, when a simple tag/branch is pushed and contains no commits, gitPush.getCommits() will return null instead of an empty list. In my opinion, this pull request has the right approach: if (commits == null || commits.size() < 1)

@olivierdagenais
Copy link
Member

I think I agree with @cniweb: this change replaces NullPointerException with IllegalArgumentException, meaning the hook would return HTTP 400 instead of HTTP 500.

It seems to me it might be best to skip the gitPush event's reaction (i.e. polling) if no commits were actually pushed, possibly also emitting an informational message to the log.

This would be implemented by GitPushEvent#determineCommit returning null if it can't find a commit and then AbstractHookEvent#pollOrQueueFromEvent checking if commit is null and adding an appropriate response to result if that's the case, kind of like the one that adds No Git jobs found.

What do you think, @CSchulz and @davidstaheli?

Thanks!
- Oli

This can apparently happen when pushing a tag only.
@olivierdagenais
Copy link
Member

@CSchulz: I have added commit cba1ab0 (which implements my above suggestion) to your branch and pushed it, effectively updating this pull request.

What do you think?

Thanks!
- Oli

@olivierdagenais
Copy link
Member

Thank you all for your help on this matter!

@olivierdagenais olivierdagenais merged commit 0bcbf31 into jenkinsci:master Dec 31, 2016
@CSchulz
Copy link
Contributor Author

CSchulz commented Jan 2, 2017

Nice idea, I was on vacation. 😎

What about rebase (force) pushes? I think they won't get processed, right?

@cniweb
Copy link

cniweb commented Jan 10, 2017

Hi @olivierdagenais,

When will a new release be released?

Chris

@olivierdagenais
Copy link
Member

When will a new release be released?

5.3.1 was released and should show up soon in the update center.

Thank you for your patience!

- Oli

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.

4 participants