Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a template string for the crictl download URL #83

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

olavst-spk
Copy link
Contributor

Pull Request (PR) description

This PR replaces the download_url parameter of the crictl class with a new parameter called download_url_template. This parameter uses template variables and is rendered using the k8s::format_url function, similar to how other download URLs are handled in this module.

The k8s::format_url function auto-detects the CPU architecture, so the arch parameter is no longer required. I am therefore removing it from the class.

NOTE: This is a breaking change for users that use the download_url parameter. Should this be noted somewhere?

This Pull Request (PR) fixes the following issues

Without templating, it is cumbersome to override the download URL. The user has to handle the version and arch themselves. It is also inconsistent with other parts of the module that use templating for download URLs.

@olavst-spk olavst-spk marked this pull request as ready for review February 16, 2024 10:57
@ananace
Copy link
Member

ananace commented Feb 16, 2024

I noted the k8s::format_url method for the PR that added the class as well, though unfortunately only after it had been merged.

Not sure if the ability to specify arch was a requirement at the time, so I'll leave @rwaffen to comment on that.

I feel like the method parameter could be useful here as well, so you can use both a direct binary url or an archive one, but that's just a personal feeling in regards to keeping things consistent - especially seeing as I don't use the crictl tooling myself. (Don't even know if direct binary download is a thing in this case)

@ananace ananace requested a review from rwaffen February 16, 2024 11:31
@rwaffen rwaffen merged commit 497e043 into voxpupuli:master Feb 20, 2024
3 checks passed
@h-haaks h-haaks mentioned this pull request Feb 21, 2024
@h-haaks h-haaks deleted the crictl_download_url branch February 23, 2024 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants