Generic provider by speier · Pull Request #179 · marcosnils/bin

speier · 2023-11-03T11:43:23Z

Started as a Helm provider, trying to turn into a generic provider, see comments below.

~~Provider to download Helm releases from https://get.helm.sh~~

~~Supports getting latest version from https://get.helm.sh/helm-latest-version or fetching specified version.~~

Unfortunately Helm using a rather strange way for their releases and they don't provide a list of release builds for OS/Arch, so we need to maintain a list of possible architecture and OS versions in the provider.

marcosnils · 2023-11-05T01:31:00Z

Hey! thx for the PR! retrieving binaries from direct http endpoints has been an idea that I've been waiting to generalize as a provider for a while and seems like this PR could be the perfect segway towards that. WDYT if we take this PR as a starting point and create a new http provider that additionally allows receiving a --lastest-url flag which basically takes care of this? I'd envision the UX something like

bin install --latest-url https://get.helm.sh/helm-latest-version https://get.helm.sh/helm-v3.13.1-linux-arm64.tar.gz

The idea is that bin will store the latest-url config and will use it for future bin update runs.

How does that sound?

speier · 2023-11-05T09:29:36Z

Hi! Yes, sounds good. I was also wondering how to have a generic approach.

I will update the PR.

speier · 2023-11-07T06:59:41Z

Did you have a chance to take a look at the updated PR commits?

MurzNN · 2023-12-11T10:16:37Z

It's a brilliant idea to provide providers with the most popular tools like helm, kubectl! Always get sad about downloading and updating them manually.

marcosnils · 2023-12-11T22:52:43Z

Did you have a chance to take a look at the updated PR commits?

hey! my apologies for delaying on this one. Will check it out this week 🙏

marcosnils · 2023-12-17T16:25:47Z

+	all       bool
+	force     bool
+	provider  string
+	latestURL string


we should move this latestURL inside the generic provider only since it doesn't apply to all of them

marcosnils · 2023-12-17T16:28:03Z

 			// and triger the update process if that's the case

-			p, err := providers.New(u, root.opts.provider)
+			p, err := providers.New(u, root.opts.provider, root.opts.latestURL)


same as the comment above. The provider itself should be the one who parses the latest url from the provided flags instead of sending this argument to all provider initializations. We'll have to refactor the code a bit but I believe it's worth it. We'll have to use different flagsets (https://pkg.go.dev/flag#NewFlagSet) per provider.

marcosnils · 2023-12-17T16:34:32Z

 				Hash:        fmt.Sprintf("%x", pResult.Hash.Sum(nil)),
 				URL:         u,
 				Provider:    p.GetID(),
+				LatestURL:   root.opts.latestURL,


Same as other comments, having this in the top level of the config seems weird. I was thinking that we could maybe extend the provider key we already have and make it map instead of string. That way, we can save provider specific configurations in the config.

We could add a new GetConfig methgod to the provider interface for this.

marcosnils · 2023-12-17T16:35:19Z


 			for _, b := range binsToProcess {
-				p, err := providers.New(b.URL, b.Provider)
+				p, err := providers.New(b.URL, b.Provider, b.LatestURL)


Same as comments in install.go

marcosnils · 2023-12-17T16:35:24Z

 			for ui, b := range toUpdate {
-
-				p, err := providers.New(ui.url, b.Provider)
+				p, err := providers.New(ui.url, b.Provider, b.LatestURL)


Same as comments in install.go

marcosnils · 2023-12-17T16:36:19Z

nit: renane generic.go to http_generic.go? WDYT?

marcosnils · 2023-12-17T16:37:59Z

+func (g *generic) GetLatestVersion() (string, string, error) {
+	log.Debugf("Getting latest release for %s", g.name)
+
+	resp, err := http.Get(g.latestURL)


we should check if latestURL is not empty here so we can add proper debug logs for this with a message like: latest url not found for $name or something

marcosnils · 2023-12-17T16:38:26Z

 )

-func New(u, provider string) (Provider, error) {
+func New(u, provider, latestURL string) (Provider, error) {


Same as comments in install.go

marcosnils · 2023-12-17T16:38:35Z

 		return newHashiCorp(purl)
 	}

+	if len(latestURL) != 0 && (provider == "generic" || len(provider) == 0) {


Same as comments in install.go

marcosnils · 2023-12-17T16:43:32Z

+	return fname, fmt.Sprintf("%s/%s", g.baseURL, fname)
+}
+
+func newGeneric(u *url.URL, latestURL string) (Provider, error) {


seems like this provider is not very generic since it expects the URL to have a specific format or otherwise it'll fail. I'll leave a more general comment about this in the issue.

marcosnils · 2023-12-17T16:50:14Z

hey @speier just found the time to start looking at this and it's looking great and in the right direction. The most relevant comment I have is that the provider doesn't seem to be generic. If I understood the code correctly, if you give it an URL with a shape different than https://get.helm.sh/helm-v3.13.1-linux-arm64.tar.gz, it'll just fail to parse it and error out.

Maybe I didn't explain my idea very well before ( apologies, not a native English speaker). My initial thought was to allow the generic provider to be able to fetch any kind of URL regardless of the format. So I could also do something like bin install https://some_url/some_file.tar.gz and it'll also work. Optionally, this provider could receive a latest-url argument that it'll use to check if there's a new version (initially, it could only check if the value is different to what's persisted in the config) and download the new file if this is the case.

LMK if that makes sense or if there's something you see I'm missing.

Thx again for contributing!

speier · 2024-04-19T05:56:14Z

hey @speier just found the time to start looking at this and it's looking great and in the right direction. The most relevant comment I have is that the provider doesn't seem to be generic. If I understood the code correctly, if you give it an URL with a shape different than https://get.helm.sh/helm-v3.13.1-linux-arm64.tar.gz, it'll just fail to parse it and error out.

Maybe I didn't explain my idea very well before ( apologies, not a native English speaker). My initial thought was to allow the generic provider to be able to fetch any kind of URL regardless of the format. So I could also do something like bin install https://some_url/some_file.tar.gz and it'll also work. Optionally, this provider could receive a latest-url argument that it'll use to check if there's a new version (initially, it could only check if the value is different to what's persisted in the config) and download the new file if this is the case.

LMK if that makes sense or if there's something you see I'm missing.

Thx again for contributing!

Hi! The idea was to use a somewhat standard naming convention: name-version-os-arch.ext otherwise not sure how it could work, if the url doesn't contain version/os/arch. Maybe I miss something here, but happy to discuss your ideas. Thx!

needleshaped · 2024-12-05T14:51:52Z

Hi! The idea was to use a somewhat standard naming convention: name-version-os-arch.ext otherwise not sure how it could work, if the url doesn't contain version/os/arch. Maybe I miss something here, but happy to discuss your ideas. Thx!

Hello! Please allow to join your discussion.

I would argue, that in case of generic provider such variables as os/arch should be at mercy of user. Otherwise we get non-generic provider. You could see another use case in #215, where the goal is to download shell scripts (like testssl).

As for version, "latestURL" variable name suggests, that we are handling "rolling" version.

Another discussible idea is how to optimize performance (save and check "Last-Modified" header, if exists?), or in worst case scenario - re-download and update (or better calculate hashsum and update only if necessary)

bot

speier added 3 commits October 2, 2023 14:32

add support GITHUB_TOKEN env var

7535e39

helm provider

34bf359

Merge branch 'marcosnils:master' into master

757d8bf

generic provider, drop helm provider

195131f

speier changed the title ~~Helm provider~~ Generic provider Nov 5, 2023

add name and link format function to generic provider

a775ef3

MurzNN mentioned this pull request Dec 11, 2023

bin install github.com/helm/helm #127

Closed

marcosnils reviewed Dec 17, 2023

View reviewed changes

Merge branch 'marcosnils:master' into master

e4b3ebe

This was referenced Dec 5, 2024

Feature request: download specific file(script) from Git, not just release #215

Open

Downloading from plain https URL #154

Open

Conversation

speier commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcosnils commented Nov 5, 2023

Uh oh!

speier commented Nov 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

speier commented Nov 7, 2023

Uh oh!

MurzNN commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcosnils commented Dec 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcosnils commented Dec 17, 2023

Uh oh!

speier commented Apr 19, 2024

Uh oh!

needleshaped commented Dec 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

speier commented Nov 3, 2023 •

edited

Loading

speier commented Nov 5, 2023 •

edited

Loading

MurzNN commented Dec 11, 2023 •

edited

Loading