Add support for artifacts and logs of workflow runs#1075
Conversation
|
@gsmet |
| try { | ||
| GitHubRequest request = root.createRequest().withUrlPath(getApiTailUrl("actions/artifacts")).build(); | ||
| return new GHArtifactsIterable(this, request); | ||
| } catch (MalformedURLException e) { | ||
| throw new GHException("Malformed URL", e); | ||
| } |
There was a problem hiding this comment.
Here's that try-catch again. I figured out why most other list* methods don't need this:
github-api/src/main/java/org/kohsuke/github/Requester.java
Lines 153 to 158 in a68d16d
This try catch appears in two places so in this case I definitely want it inside the iterable.
There was a problem hiding this comment.
I knew you would say that but the query is different in both cases, thus why I haven't changed it as for the other PR. So should I pass the builder instead of the request and build the request in the iterable?
Frankly, for me the issue is with the initial design decision of throwing low value unchecked exceptions from nearly everywhere.
There was a problem hiding this comment.
I knew you would say that but the query is different in both cases, thus why I haven't changed it as for the other PR. So should I pass the builder instead of the request and build the request in the iterable?
Yeah, pass the builder. Or maybe the right thing is to make a version of the build() that wraps those exceptions and use that in all 3 (4?) calls. I would change the behavior of build() but that could result in client that understand how to handle certain exceptions suddenly not getting those anymore.
Frankly, for me the issue is with the initial design decision of throwing low value unchecked exceptions from nearly everywhere.
Absolutely. Please file an issue and describe how it should work in the next version, or any improvements we can safely make now. I'm interested. Some of the current behavior is probably cruft from long removed code. It may also be a result of the underlying implementation of HttpConnector and HttpUrlConnection not being controlled by this library. okhttp3 has notably different behavior from the Java built-in implementation.
There was a problem hiding this comment.
OK, for now, I passed the builder to the iterable.
I think more involved changes need to be thought out a bit more. I'll think about it and open an issue.
|
OK, things look significantly better now. The logs tests are passing. I need to add tests for the artifacts now. I'll probably do that this evening. @bitwiseman I'll wait for your feedback on the |
gsmet
force-pushed
the
more-workflows
branch
3 times, most recently
from
66d4d3f to
50b5396
Compare
|
@bitwiseman this is now complete - I implemented all the features I wanted and the test coverage should be OK. Let me know what you want me to do for the Feel free to ask other adjustments too :). |
|
All green! |
|
Thanks! As usual, interested by a release when you get the time. We want to put this to good use :). |
3 participants
@bitwiseman I need your wisdom. The logs and artifacts are downloaded from another host (
https://pipelines.actions.githubusercontent.com/) and the way it's done currently, this call is not mocked properly.See https://github.com/hub4j/github-api/pull/1075/files#diff-87252b4de52f4e0e1317a91eee8a0de1cc885f7b84f872989c8e52ec09d2509dR296 for an example of how it is currently done.
I'm unsure of how we could handle this and thought I might as well ask.
Note: the PR is not completed yet, it's still missing tests for the artifacts. I only test the logs method for now.
works but running the test after that doesn't work as the actual download call wasn't mocked.