Add helper download.InMemory for downloading files without writing to disk

[anjannath]

this helper is put in a separate package pkg/download/inmemory to avoid import cycles which occurs due to the network package being imported in the pkg/download package, the cycle is network->ssh->constants->version as a result we'll not be able to import pkg/download in pkg/crc/version

currently the helper is used for downloading release-info.json file and the signed bundle hashes file sha256sum.txt.sig

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from anjannath. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[cfergeau]

this helper is put in a separate package pkg/download/inmemory to avoid import cycles which occurs due to the network package being imported in the pkg/download packag

This import cycle already caused issues in the past, I think another way of breaking the cycle is by moving the proxy code (HTTPTransport()) to its own package

[cfergeau]

You can avoid the use of the generics by always setting the user-agent in this method, which makes sense in my opinion.

[anjannath]

i think in zabbix we ignore requests with http agent crc * from the download data count, with the current use of this (for getting release info and bundle hashes) it should be okay, will remove the type parameters and set the user-agent in the function itself

[anjannath]

more import cycles, to set the user agent to crc/<crc_version>we have to import the version package in inmemory package and the version package has to import the inmemory package to download the release-info.json

[cfergeau]

We still want to be consistent imo and always use the same user agent. I guess for now you could add a userAgent string parameter to Download, and a version.UserAgent(), and pass it explicitly. Otherwise this needs more code movement.

[gbraad]

does it functionally change anything? what are you planning to do with the 'useragent'; track it as telemetry?

[cfergeau]

Downloading the crc version file is done with user agent set, downloading the signature file is done without a user agent set, and there are 2 code paths in the download code to deal with both situations.
I don't mind dropping the user agent or always setting it, but it would be better to always do the same thing.

See also #3693 (comment)

[anjannath]

When we request the release-info.json file from the CRC codebase to check if a new version exists, we set the User-Agent for that call as, e.g crc 2.20.0 this was added to make sure that we are not skewing the download counts by our own tool making http requests to the download location.

and in zabbix where this download data is collected filters the requests having user agent crc * so these are not added to the download count

[anjannath]

User-Agent seems is not used to filter out http requests from crc, not sure how i reached that, but it doesn't make sense also as for collecting the download data from splunk we can use a query language and can filter out the release-info.json or the sha256sum.txt.sig files.

[gbraad]

I think another way of breaking the cycle is by moving the proxy code (HTTPTransport()) to its own package

is this the alternative you prefer or is this something that can ALSO be done.

[cfergeau]

I think another way of breaking the cycle is by moving the proxy code (HTTPTransport()) to its own package

is this the alternative you prefer or is this something that can ALSO be done.

That's merely an alternative suggestion which can be ignored or done, no strong feeling. However, it's at least the 3rd time we get import cycles because of HTTPTransport being in the network package (happened to me in some experiments, quite sure Praveen saw that in one of his PRs, but avoided the cycle differently, now Anjan hits it too). So we could try to solve this once and for all.

[anjannath]

i haven't looked at solving this in a better way so not sure how much work it'll be, created #3697 for planning/tracking

[cfergeau]

Looks good to me, there are several mentions of inmemory.Download in the commit logs which no longer are accurate.
You should also mention in the last commit log that this should help with bug xxx (the one with the download timeouts).

[anjannath]

Looks good to me, there are several mentions of inmemory.Download in the commit logs which no longer are accurate.
You should also mention in the last commit log that this should help with bug xxx (the one with the download timeouts).

Thanks, updated the commit logs!

[anjannath]

Changed the function name to download.InMemory, as otherwise, gorevive complains about the name..

Error: pkg/download/download.go:91:6: exported: func name will be used as download.DownloadInMemory by other packages, and that stutters; consider calling this InMemory (revive)
func DownloadInMemory(url, userAgent string, transport http.RoundTripper) (io.ReadCloser, error) {
     ^