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

Add basic checksum #1

Merged
merged 4 commits into from
Oct 25, 2015
Merged

Add basic checksum #1

merged 4 commits into from
Oct 25, 2015

Conversation

catsby
Copy link
Contributor

@catsby catsby commented Oct 19, 2015

This adds a basic checksum feature to go-getter. It works by checking a checksum supplied as a query parameter:

if err := gets.GetFile("local.jar", "https://someurl/demoapp.jar?checksum=58d6e8130308d32e197c5108edd4f56ddf1417408f743097c2e662df0f0b17c8"); err != nil {
    log.Fatalf("\nFatal: %s\n", err)
}

Alternatively, we could change checksum param to sha256, and add support for md5 et. al

@mitchellh
Copy link
Contributor

This looks good! We're gonna need a couple things though:

  • Unit tests
  • Support for every other checksum type (md5, sha1, etc.). I know from experience implementing checksums with Packer that this will be requested very early.

@cbednarski
Copy link

Can we specify the checksum in some way that doesn't modify the URL, since that might have side effects on the system we're downloading from?

This might be easier to add if GetFile() accepts a struct like:

type GetFileOptions struct {
    Source string
    Target string
    HashType string
    HashValue string
}

We could also add auth to this struct later and not need to change the function signature.

@mitchellh
Copy link
Contributor

@cbednarski We can't do that.

It is idiomatic "go-getter" to use the URL. The URL is never passed as-is to the underlying system. We parse magic query parameters out or URLs for various things (see "ref" with Git URLs). But, you do bring up a good point that we should remove it from the URL so that it isn't part of it anymore.

The reason we can't support extra struct configs is because systems like Otto and Terraform don't have any other configuration other than URL for imports. Query parameters are meant to encode all the config and so go-getter uses the URL in its entirety for configuration.

You can see how we mangle URLs in all sorts of ways for configuration on the Otto docs page as an example (same as Terraform): https://ottoproject.io/docs/appfile/dep-sources.html

@cbednarski
Copy link

It is idiomatic "go-getter" to use the URL. The URL is never passed as-is to the underlying system. ... Query parameters are meant to encode all the config and so go-getter uses the URL in its entirety for configuration.

Ah that makes sense; I see how that works from the Otto examples. Thanks for clarifying!

@catsby catsby mentioned this pull request Oct 23, 2015
5 tasks
@avnik
Copy link

avnik commented Oct 24, 2015

Specifying http://url/to/file#md5=hash looks more sane, than HTTP GET ?checksum=... parameter. And it common practice used by python.org for years

@mitchellh
Copy link
Contributor

@avnik For the same reason as my response to @cbednarski above, I don't think it'll matter. Plus, it is more idiomatic of this library to use query parameters. It is our API, so to speak. :) We don't parse out hash params, although I agree that in practice in links this is idiomatic.

@mitchellh
Copy link
Contributor

I made the changes to support all checksums plus added tests!

@catsby Feel free to use as a reference, since you just get started with the project I wanted to give you some guidance without having you blocked on this.

mitchellh added a commit that referenced this pull request Oct 25, 2015
@mitchellh mitchellh merged commit f3864c8 into master Oct 25, 2015
@mitchellh mitchellh deleted the f-checksum branch October 25, 2015 03:07
aliscott added a commit to aliscott/go-getter that referenced this pull request Dec 19, 2024
hashicorp#1)

This is useful for module references like: `github.com/terraform-aws-modules/terraform-aws-rds//modules/db_instance`,
and means the whole repository doesn't need to be pulled if just a subdirectory is needed.

This doesn't handle any references within that submodule. For example, if it references modules located in other directories in the repo or if it symlinks into any other directories in the repo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants