Try resolve commit from run first #27
Conversation
|
|
||
| private String resolveCommit() { | ||
| if (run == null) { | ||
| return StringUtils.EMPTY; |
There was a problem hiding this comment.
returning an empty string is usually a codesmell, just return null if you can't find it, imo
| return StringUtils.EMPTY; | |
| return null; |
There was a problem hiding this comment.
Then we need to guard all methods that invoke the getSha.
There was a problem hiding this comment.
This method is only called internally from one place?
| return resolveCommit(revision.get()); | ||
| } | ||
| else { | ||
| return StringUtils.EMPTY; |
There was a problem hiding this comment.
| return StringUtils.EMPTY; | |
| return null; |
There was a problem hiding this comment.
Maybe it makes sense to also have a look at
#26
before we merge this change.
In my PR I found several paths where the context will throw an exception. Shouldn't we check before we create a publisher if such an exception will occur (e.g., if there is no SHA?).
| @@ -75,7 +76,13 @@ | |||
| * @return the commit sha of the run or null | |||
There was a problem hiding this comment.
If null is allowed (which I think is a bad choice) then we must mark the method with Nullable
| } | ||
|
|
||
| private String resolveCommit() { | ||
| if (run == null) { |
There was a problem hiding this comment.
In my refactored version run is not a field anymore and thus cannot be null. Rather than setting run to null we should immediately initialize all fields in the constructor. Then we do not have any null checks for run anymore.
I'm not sure if I did understand the GitHub REST API correctly: is it valid to call it without a SHA? If not then we should not create a ChecksPublisher. This is the way I did it with Freestyle jobs and pipelines.
There was a problem hiding this comment.
The reason we introduce the nullable run here is that when the job is still in the queue, the run is not available, so we can't get the url of the run (see #13 (comment)) and will return the url of the job view instead.
There was a problem hiding this comment.
I didn't find a better way to avoid nullable run, but maybe we can add another boolean field isLastBuild when constructing the context to indicate whether the run is the last build? So when isLastBuild == true, we get run by job.getLastBuild(), when isLastBuild == false, we use the run that was passed in the constructor.
There was a problem hiding this comment.
I saw your refactoring on this problem, simple but great! Ignore the previous two comments :)
There was a problem hiding this comment.
I'm not sure if I did understand the GitHub REST API correctly: is it valid to call it without a SHA? If not then we should not create a ChecksPublisher. This is the way I did it with Freestyle jobs and pipelines.
The SHA is required for a check, without it, the check cannot be created successfully.
|
|
||
| private String resolveCommit() { | ||
| if (run == null) { | ||
| return StringUtils.EMPTY; |
There was a problem hiding this comment.
Then we need to guard all methods that invoke the getSha.
| } | ||
| else { | ||
| throw new IllegalStateException("Unsupported revision type: " + revision.get().getClass().getName()); | ||
| throw new IllegalStateException("Unsupported revision type: " + revision.getClass().getName()); |
There was a problem hiding this comment.
Shouldn't we catch that error before we create the publisher and return then a NullPublisher?
checkoutinstead ofonStartedsincecheckoutis just afteronStartedand commit is available in run aftercheckout