search

taiki-e / install-action

GitHub Action for installing development tools (mainly from GitHub Releases).
Apache License 2.0
246 stars 32 forks source link

Use asset etag to avoid fetches #547

Closed jayvdb closed 3 weeks ago

jayvdb commented 3 weeks ago

c.f. https://github.com/taiki-e/install-action/issues/540#issuecomment-2170998595

This doesnt reduce the number of API hits - it only halts downloads if the current etag is the same as in the head of the asset fetch.

As a result the manifest CI job goes from 4m30s to ~2m.

Using If-None-Match header would also work, but

  1. that would add complexity to download(..), and the logic around the invocation to optionally provide an etag if it was found in the existing manifest.
  2. I found using head doesnt work, as the Server responded to ureq::head with 401 if If-None-Match is used; however responds to wget head with 304.

IMO it would be worthwhile adding complexity to download(..) if more of the fetches also become etag capable, which I believe is possible and worthwhile, but IMO not as valuable as the asset fetches because they are so numerous and voluminous to be likely to prevent features being developed by new contributors who will balk at downloading so much data just to check a new feature against the full set of manifests.

The other major addition is writing the manifest during processing of many versions - some of the larger manifests were failing when I was downloading all of the old assets, and needed to be restarted, and I was loosing too much data/time if it had to restart at the beginning with all the etags missing.

jayvdb commented 3 weeks ago

Note this PR is broken by every manifest update (and is broken now), and it is not trivial to update it, because the updated manifest on main cant be parsed by serde because the etag is missing.

taiki-e commented 3 weeks ago

Sounds good, let's merge this and try it out.