YETUS-1060. github status should use htmlreport #261
Conversation
|
Asking @aajisaka to review since this feature request essentially came from him during some other PRs. |
ndimiduk
left a comment
ndimiduk
left a comment
There was a problem hiding this comment.
Some small questions, looks alright to me otherwise. Should be a nice enhancement!
|
|
||
| ## HTML Details LInk | ||
|
|
||
| By default, if Apache Yetus is under conditions when it would normally write a Github Status (e.g., Jenkins processing |
There was a problem hiding this comment.
Can you show an example of the current behavior and (if possible) the htmlreport that will be the new default after this patch? I'm looking at an HBase PR (Yetus running in the context of an Jenkins Multi-branch plugin) and I don't see a "Details" link anywhere in the comment left by the Jenkins build worker. Perhaps my example context doesn't apply to this change?
There was a problem hiding this comment.
Or perhaps my sample PR doesn't apply at all because HBase isn't yet using the GitHub Checks API...
There was a problem hiding this comment.
Correct. That PR isn't using the checks API. You can see in pretty much any Apache Hadoop PR that down at the bottom there is a 'Details' link for 'Apache Yetus'. That should now point to the HTML report instead of the Jenkins console post this PR.
There was a problem hiding this comment.
Apache Yetus 0.12.0
That PR isn't even checks API capable...
| # - failure w/log | ||
| # - success w/warning log | ||
|
|
||
| url=$(get_artifact_url) |
There was a problem hiding this comment.
Minor nit: why move population of the url value? Does the artifact_url change from the beginning of this function call to down here?
There was a problem hiding this comment.
It doesn't and this change was a result of some of the development work that happened previously.
| { | ||
| declare result=$1 | ||
|
|
||
| rm "${commentfile}" 2>/dev/null |
There was a problem hiding this comment.
Why rm with error output ignored instead of something like rm -f ?
There was a problem hiding this comment.
It might not exist due to a previous failure elsewhere. That's ok and we don't want the error to go to console.
| { | ||
| declare result=$1 | ||
|
|
||
| rm "${commentfile}" 2>/dev/null |
There was a problem hiding this comment.
Should you check the exist code of rm and handle failure?
There was a problem hiding this comment.
Failure here is OK to ignore.
|
I'm going to go ahead and merge this one. Thanks for the review @ndimiduk ! |
|
Sorry for being late. I'll test this in Apache Hadoop. |
|
Tested and it looks good to me. Thanks. |
3 participants
No description provided.