Skip to content

Create sub modules cmd/go-getter/v2, s3/v2 and gcs/v2#244

Merged
azr merged 2 commits intov2from
submodules
May 11, 2020
Merged

Create sub modules cmd/go-getter/v2, s3/v2 and gcs/v2#244
azr merged 2 commits intov2from
submodules

Conversation

@azr
Copy link
Copy Markdown
Contributor

@azr azr commented Apr 20, 2020

This PR creates the following submodules for the go-getter:

.
├── cmd
│   └── go-getter
├── gcs
└── s3

Some testing helpers were used everywhere so I put them in "helper/testing". I also added the "helper/url".MustParse function that is used everywhere. Other than that what's unexported is still unexported.

This PR replaces #238 so that we can see circle-ci's results and this uses #241 too to avoid divergences.

close #193

@azr azr force-pushed the submodules branch 3 times, most recently from c499020 to d7c92ef Compare April 20, 2020 14:40
Comment thread cmd/go-getter/main.go

client := getter.DefaultClient

getters := getter.Getters
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a change from the current behavior, where callers can define its own list of getters (client.Getters) and only those clients get set (originally in client_options.go, when it iterates over the getters if not nil). Am I misreading this change?

Here's an example from terraform: https://github.com/hashicorp/terraform/blob/807267d1b56aa6298fecf5edba98c088c2866e83/internal/initwd/getter.go#L131

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the v2 branch the client was split from the request and the getters are set on the client, so here this line from terraform (link) could become:

var goGetterClient = getter.Client{
	Getters: map[string]getter.Getter{
		"file":  new(getter.FileGetter),
		"gcs":   new(getter.GCSGetter),
		"git":   new(getter.GitGetter),
		"hg":    new(getter.HgGetter),
		"s3":    new(getter.S3Getter),
		"http":  getterHTTPGetter,
		"https": getterHTTPGetter,
	},
}

Copy link
Copy Markdown
Contributor Author

@azr azr Apr 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the call would be : client.Get(ctx, getter.Request{ /*...*/ })

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it now, thank you! Definitely a misread on my part.

@mildwonkey
Copy link
Copy Markdown

This looks fine by me (with my "terraform core" hat on)! Since there's a (tiny) breaking change I would like to see that documented or in the changelog, but that's not blocking this PR. Cheers 🎉

@azr
Copy link
Copy Markdown
Contributor Author

azr commented Apr 21, 2020

Thanks for your review. Ah I totally forgot about the changelog; I'll draft a PR right away for the whole v2 branch, once this is merged.

Copy link
Copy Markdown

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice!! I just made a question because you removed something that I use in #240.

Comment thread appveyor.yml Outdated
Comment thread gcs/get_gcs.go
// a GCS bucket.
type GCSGetter struct {
getter
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove this? 🤔
I'm asking because the SmbGetter is using this to check if there's an available FileGetter. We need to check that In case the user doesn't want to provide the FileGetter and then SmbGetter can't use it.

Copy link
Copy Markdown
Contributor Author

@azr azr Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good question, I removed it because it is unused and is a bad pattern.

I initially added it in the master branch where the client/request are the same object. It would have allowed to reuse the getters of a request to do things like downloading the checksum from a file. It turns out it was not used.

I think this pattern was pretty bad and was going away from the initial design of this library which was aiming at avoiding a getter from knowing about the client ( so separating the concerns ).

Now if the SmbGetter needs this; let's pause a minute to think a little more; I need to re-read your PR with that in mind.

Comment thread module_test.go
@azr azr force-pushed the submodules branch 5 times, most recently from 6f037bb to b9074f1 Compare April 28, 2020 15:23
to not force importers to import 3rd party getters and dependencies
@azr
Copy link
Copy Markdown
Contributor Author

azr commented May 11, 2020

Okay, merging this one ! Thanks everyone, we are not quite ready to tag this yet so there's probably time to fix/change things around.

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.

3 participants