-
Notifications
You must be signed in to change notification settings - Fork 724
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
Add support for downloading zip and tar archives of repositories. #967
Conversation
Item 2 will require an update to the test framework to fix, but it should be pretty easy - adding another mock server to the list and then recording. You can do that as part of this PR if you like. Looks like there is another PR that needs to do something similar. As an alternative, you could record the interaction and then manually create a payload that points to the |
} | ||
|
||
private void downloadArchive(String type, Optional<String> ref, StreamConsumer sink) throws IOException { | ||
requireNonNull(sink, "Sink must not be null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use spotbugs annotation for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the annotation doesn't actually enforce it at run time though as far as I know?
they provide hints for IDE and static analysis tools
* @throws IOException | ||
* The IO exception. | ||
*/ | ||
public void zipball(StreamConsumer sink, String ref) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see why you did this. This is the endpoint you're hitting, but maybe we should use a more Java-like name for the methods? I'm interested to know your opinion.
public void zipball(StreamConsumer sink, String ref) throws IOException { | |
public void downloadZip(StreamConsumer sink, String ref) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this with new library-wide functional interfaces. Also fixed the codeload
and redirect issue.
The main question I have is whether it is better to have the public readZip
and readTar
methods take a Consumer
or a Function
. This matters because implementing one now and adding the other later may result in compilation errors - the compiler may not be able to distinguish between the two types for some lambda inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure either way on whether consumer of function is better
} | ||
|
||
private void downloadArchive(String type, Optional<String> ref, StreamConsumer sink) throws IOException { | ||
requireNonNull(sink, "Sink must not be null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the annotation doesn't actually enforce it at run time though as far as I know?
they provide hints for IDE and static analysis tools
@timja Thanks for taking the time to review. |
2367e42
to
936ab49
Compare
👋, found myself needing to download zip/tar archives of repository content which wasn't supported so mucked about with getting that in place.
Now, there's a few things to note here which I'm not sure if/how to tackle:
302
redirect to the actual archive which is hosted athttps://codeload.github.com/<owner>/<repo>/legacy.<tar.gz|zip>/<ref>
but the redirect doesn't seem to be captured properly by wiremock and the download itself isn't captured at all even when adding manual logic to follow the redirectRequester.client
in order to be able to stream the content directly into aStreamConsumer
sinceRequester.fetchStream()
reads everything into memory which didn't seem ideal for dealing with entire repository archives