Refactor and expand download_hash.py

[VannTen]

What type of PR is this?
/kind feature

What this PR does / why we need it:
This reworks the Python version of the download script making it much more
powerful and versatile and supporting most of the components we download.

It's intended to be the first step in completely replacing download_hash.sh.
It's missing some things for that (see 86855be) that I will address in later PRs.

It has the following advantages over download_hash.sh in particular:

• which versions and which archs to download for each components is driven by the current content of the downloads variable, which allow to avoid lots of '0' value.
• It only downloads hashs; I intend to keep it that way unless absolutely necessary, as downloading the binary locally is super wasteful.
  This means it will have to handle different hash format, and crypto signing, certainly (gpg + cosign for k8s binaries).
  I've also started asking upstream project which don't provide any of these if they would kindly do so (that's just youki for now)
• it's faster. It's not (yet) a fair comparison (because some components are not handled), but this is a result for a full run of both on a clean current version of master:

$ time python download_hash.py

...

real 1m26.888s
user 0m0.827s
sys 0m0.058s

$ time python download_hash.sh

...

real 14m32.278s
user 0m41.258s
sys 0m29.733s

Special notes for your reviewer:
My python code is pretty functional (in the Haskell sense, lambdas and
comprehensions all other the place), you've been warned ^.

I didn't mark this as draft because it does not affect end-users, but there is still things I'm unsure about:

1. Handling '0' / 'NONE' (see the help text for the current semantics)
2. Is the documentation clear enough for using it easily ?

Does this PR introduce a user-facing change?:

NONE

/label tide/merge-method-merge
/cc @MrFreezeex
/cc @mzaian

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VannTen

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [VannTen]

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

[VannTen]

/ok-to-test

[VannTen]

/cherrypick release-1.26 /cherrypick release-1.25 /cherrypick release-1.24

[k8s-infra-cherrypick-robot]

@VannTen: once the present PR merges, I will cherry-pick it on top of release-1.26 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.26
/cherrypick release-1.25
/cherrypick release-1.24

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[MrFreezeex]

Here is an initial review from a first look, overall I agree with the direction this is going but I left some comments to improve readability inline. Thanks!

[MrFreezeex]

Could these generators be independent functions? it would probably be much more readable that way IMO

[VannTen]

Hum, I might be able to convert this two loops into one, since I don't use minor of patches expect in the sub-loop, and then put that in a distinct fonction.

[MrFreezeex]

Having such complex expression directly in the for loop makes it super hard to read IMO, could you refactor this in a few lines before the for loop?

[VannTen]

You mean something like for instance ?

Suggested change

	for download, url in (downloads if only_downloads == []
	else {k:downloads[k] for k in downloads.keys() & only_downloads}).items():
	if only_downloads != []:
	downloads = {k:downloads[k] for k in downloads.keys() & only_downloads}
	for download, url in downloads.items():

[MrFreezeex]

yep 👍

[MrFreezeex]

btw maybe you have a good reason for it but in general I think this:

if not only_downloads:

instead of:

if only_downloads != []:

is slightly more idiomatic (although it's not technically checking the same thing strictly speaking) but that's more a nit feel free to do what you prefer

[VannTen]

Not a specific reason, it's just I tend to avoid "implicit conversion to boolean" even in Python, I find it confusing (for later)

[MrFreezeex]

I feel like there is some unnecessary duplicated code here and you could find a way to do the file download/error checking in a common function and/or still rely on _get_hash_by_arch and handle the if download in download_hash_extract there somehow

[VannTen]

This comes from the need to cache memoize _get_hash_by_arch (to avoid redundant HTTP calls). functools.cache memoize by parameters, so I can't pass arch to _get_hash_by_arch because it would break the memoization. OTOH, I need it for "simple hash" download .

I could extract the GET + check 404 + raise in it's own little function though 🤔

[MrFreezeex]

Ahh ok I see, hmm well if you find some elegant way to not duplicate this piece of code that could be nice but otherwise feel free to resolve that comment then

[VannTen]

I'm also looking if I can just drop all the "guess version and handle 404" by leveraging the GitHub graphql api to get a complete list of the versions needed (with a single api call thanks to graphql). So far it appears promising, so I may end up dropping the unreadable generators all together o/

[yankay]

Thanks @VannTen

Greate Refactor
/lgtm

[k8s-infra-cherrypick-robot]

@VannTen: cannot checkout release-1.26: error checking out "release-1.26": exit status 1 error: pathspec 'release-1.26' did not match any file(s) known to git

In response to this:

/cherrypick release-1.26
/cherrypick release-1.25
/cherrypick release-1.24

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[k8s-infra-cherrypick-robot]

@VannTen: cannot checkout release-1.25: error checking out "release-1.25": exit status 1 error: pathspec 'release-1.25' did not match any file(s) known to git

In response to this:

/cherrypick release-1.26
/cherrypick release-1.25
/cherrypick release-1.24

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[k8s-infra-cherrypick-robot]

@VannTen: cannot checkout release-1.24: error checking out "release-1.24": exit status 1 error: pathspec 'release-1.24' did not match any file(s) known to git

In response to this:

/cherrypick release-1.26
/cherrypick release-1.25
/cherrypick release-1.24

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[tico88612]

/cherrypick release-2.26
/cherrypick release-2.25
/cherrypick release-2.24

[k8s-infra-cherrypick-robot]

@tico88612: new pull request created: #11538

In response to this:

/cherrypick release-2.26
/cherrypick release-2.25
/cherrypick release-2.24

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[tico88612]

/cherrypick release-2.25

[k8s-infra-cherrypick-robot]

@tico88612: new pull request created: #11539

In response to this:

/cherrypick release-2.25

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.