search

abdolence / sbt-gcs-resolver

SBT plugin for Google Cloud Storage (GCS) and Google Artifact Registry with Coursier support
Apache License 2.0
28 stars 7 forks source link

Head request fetched when connecting #71

Closed peterrosell closed 5 months ago

peterrosell commented 6 months ago

I'm wondering what the purpose of the HEAD request in the connect() method. When using a custom remote repository in GAR the HEAD request results in a GET request. So the current solution, version 1.10.0 do two request to the upstream repositories.

I see that it sets the responsecode and responseMessage if the head request fails. For me it seems to be a bit confusing if the code that uses the plugin reads these values in different scenarios.

In the download scenario it should work, even if I think the upstream should be connected when the connect() method is called and when the getInputStream method is called the inputStream from the upstream request should be returned. This might make the getInputStream method easier as the caching will be handled by the upstream connection. This will also remove the head request as it's not necessary anymore.

In the upload scenario the head request in the connect() is a bit odd as the artifact normally doesn't exist before it's uploaded and will always set the responseCode and responseMessage. These values are never changed after the connect() methods and might be confusing for the code that use the plugin.

I had a look at Google's maven wagon extension and there is a openInternalConnection method that don't do any connection, only prepare the factory. In the getInputStream method the request is created and its inputStream is returned.

Based on this I think the HEAD request can be removed in the connect method.

And once again thanks for building this plugin. :heart:

abdolence commented 6 months ago

I'm wondering what the purpose of the HEAD request in the connect() method. When using a custom remote repository in GAR the HEAD request results in a GET request. So the current solution, version 1.10.0 do two request to the upstream repositories.

This plugin has long history :) I don't remember now all details, but it was used before to check if file exists and its size. For Ivy it is still used this way. I don't think for downloading it is an issue, since HEAD seems supported fine by GAR.

In the upload scenario the head request in the connect() is a bit odd as the artifact normally doesn't exist before it's uploaded and will always set the responseCode and responseMessage. These values are never changed after the connect() methods and might be confusing for the code that use the plugin.

This is why I removed explicit call in upload() in v1.10.0. It was just a copy paste bug for upload, and of course didn't make sense too much (only if we wanted to check if we're overwriting something).

Again I'm open to PRs if you have time to remove it and test it that it doesn't affect anything :)

abdolence commented 6 months ago

Thanks for the feedback, help with this and kind words!

peterrosell commented 6 months ago

I will have a look next week. If you look at the connect() code in my old PR you see that I did that change and it worked good.

peterrosell commented 5 months ago

Are you planing to make a new release soon? I would like to reduce our load on GAR. :)

abdolence commented 5 months ago

Are you planing to make a new release soon? I would like to reduce our load on GAR. :)

Haha, I don't think if Google even notices this ever, but here you go: https://github.com/abdolence/sbt-gcs-resolver/releases/tag/v1.11.0

It is usually requires some time until maven repo caches contain the new ver, but I did my part :)

peterrosell commented 5 months ago

Thanks a lot!
We have had some problems that might been related to hitting the quota. Maybe on upstream remote repos, but it has been hard to pinpoint. It has got better with the new version and now I also added caching of the fetched artifacts so the problem has reduced, but I like to optimize. :joy: