Base ISO cache file name on the full ISO URL #18723
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: albertofaria The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @albertofaria. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
k8s-ci-robot
added
the
size/XS
|
Can one of the admins verify this patch? |
|
@albertofaria do you mind elaborating with an example what you are trying to improve ? maybe share with a output example |
|
A colleague reported that a stale cached ISO was being used after changing the value of the --iso-url option. This is because both the previous and new URLs given with --iso-url referenced GitLab package artifacts, which always end in a /download component. They worked around this by manually deleting ~/.minikube/cache/iso/amd64/download. |
albertofaria
force-pushed
the
cache-full-url
branch
from
3d22393
to
abc0d3b
Compare
|
@medyagh I added more details to the commit message. |
|
Hi @albertofaria, I understand the point of the fix, but fully changing the name will make it hard to know the contents of the ISO cache dir for the average user. ie. What version is Could we maybe append it to the existing version so we can still identify the version for the default ISO file names |
|
That's a good point. How about |
|
That seems good, the 137 chars is a bit extreme, 40 sounds a lot better |
albertofaria
force-pushed
the
cache-full-url
branch
from
abc0d3b
to
ee9072f
Compare
Currently, only the final component of the URL path is used. This means that minikube may use the wrong cached image after the user changes the ISO URL to another with the same final component. Fix this by suffixing the cache file name with a hash of the entire URL. Signed-off-by: Alberto Faria <[email protected]>
albertofaria
force-pushed
the
cache-full-url
branch
from
ee9072f
to
cac76de
Compare
| @@ -77,7 +78,10 @@ func localISOPath(u *url.URL) string { | |||
| return u.String() | |||
| } | |||
|
|
|||
| return filepath.Join(detect.ISOCacheDir(), path.Base(u.Path)) | |||
| urlHash := sha1.Sum([]byte(u.String())) | |||
| fileName := fmt.Sprintf("%s-%040x", path.Base(u.Path), urlHash) | |||
There was a problem hiding this comment.
This makes the iso file name a little bit ugly to support questionable API of a single application. The common way to publish downloads is to provide a URL ending with the filename, so the current behavior makes sense.
Since the image name always end with .iso or .ISO, and the file name is likely to be in the URL even in gitlab, can be search back the URL components and take the first component ending with .(iso|ISO) as the file name?
Or, maybe apply this behavior only if the URL does not end with .(iso|ISO)? This way this change helps gitlab users, without affecting other users.
Also did you open a bug for gitlab to provide a better URL? This /download URL will cause trouble for other tools, for example what do you get with curl -O?
There was a problem hiding this comment.
Another way to solve this, add --iso-filename option. When set, using --iso-url will store the iso using the filename specified in --iso-filename. With this if you download the iso from a system that does not provide the common url with meaningful file name at the end, you can control the stored file name.
Currently, only the basename of the URL path is used, which can be the same for different URLs.
Base the cache file name on the full URL instead, encoding it using Go's base64.URLEncoding so it can be used as a file name on all platforms.