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.

Sign up for GitHub

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

Jump to bottom

fix(storage): preserve supplied endpoint's scheme #4609

Merged
merged 7 commits into from Aug 13, 2021
Merged

fix(storage): preserve supplied endpoint's scheme #4609

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
954b8f0
fix(storage): remove unnecessary variable
BrennaEpp Aug 13, 2021
439d22d
Merge branch 'master' of https://github.com/BrennaEpp/google-cloud-go…
BrennaEpp Aug 13, 2021
4f6b1dc
fix(storage): preserve supplied endpoint's scheme
BrennaEpp Aug 13, 2021
7ba574e
Merge branch 'master' into preserve-endpoint-scheme
BrennaEpp Aug 13, 2021
1892ada
Merge branch 'master' into preserve-endpoint-scheme
BrennaEpp Aug 13, 2021
6d00fbc
Add check for no scheme
BrennaEpp Aug 13, 2021
4247914
Merge branch 'preserve-endpoint-scheme' of https://github.com/BrennaE…
BrennaEpp Aug 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion storage/storage.go
View file
Open in desktop
Expand Up @@ -139,12 +139,13 @@ func NewClient(ctx context.Context, opts ...option.ClientOption) (*Client, error
if err != nil {
return nil, fmt.Errorf("storage client: %v", err)
}
// Update readHost with the chosen endpoint.
// Update readHost and scheme with the chosen endpoint.
u, err := url.Parse(ep)
if err != nil {
return nil, fmt.Errorf("supplied endpoint %q is not valid: %v", ep, err)
}
readHost = u.Host
scheme = u.Scheme
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we set scheme in L114 & L123 above-- I guess this no longer does anything?

Also, is it possible for the returned endpoint from htransport.NewClient to have an empty scheme?

I'm thinking we should keep L114 & L123 as is and only set scheme here if u.Scheme != "", just out of caution. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The endpoint for htransport.NewClient should always have a scheme as long as we send in the WithDefaultEndpoint option. But yes that makes sense, since it does not actually promise to.


return &Client{
hc: hc,
Expand Down
27 changes: 24 additions & 3 deletions storage/storage_test.go
View file
Open in desktop
Expand Up @@ -1252,38 +1252,59 @@ func TestAttrToFieldMapCoverage(t *testing.T) {
func TestWithEndpoint(t *testing.T) {
originalStorageEmulatorHost := os.Getenv("STORAGE_EMULATOR_HOST")
testCases := []struct {
desc string
CustomEndpoint string
StorageEmulatorHost string
WantRawBasePath string
WantReadHost string
WantScheme string
}{
{
desc: "No endpoint or emulator host specified",
CustomEndpoint: "",
StorageEmulatorHost: "",
WantRawBasePath: "https://storage.googleapis.com/storage/v1/",
WantReadHost: "storage.googleapis.com",
WantScheme: "https",
},
{
desc: "With specified https endpoint, no specified emulator host",
CustomEndpoint: "https://fake.gcs.com:8080/storage/v1",
StorageEmulatorHost: "",
WantRawBasePath: "https://fake.gcs.com:8080/storage/v1",
WantReadHost: "fake.gcs.com:8080",
WantScheme: "https",
},
{
desc: "With specified http endpoint, no specified emulator host",
CustomEndpoint: "http://fake.gcs.com:8080/storage/v1",
StorageEmulatorHost: "",
WantRawBasePath: "http://fake.gcs.com:8080/storage/v1",
WantReadHost: "fake.gcs.com:8080",
WantScheme: "http",
},
{
desc: "Emulator host specified, no specified endpoint",
CustomEndpoint: "",
StorageEmulatorHost: "http://emu.com",
WantRawBasePath: "http://emu.com",
WantReadHost: "emu.com",
WantScheme: "http",
},
{
desc: "Endpoint overrides emulator host when both are specified - https",
CustomEndpoint: "https://fake.gcs.com:8080/storage/v1",
StorageEmulatorHost: "http://emu.com",
WantRawBasePath: "https://fake.gcs.com:8080/storage/v1",
WantReadHost: "fake.gcs.com:8080",
WantScheme: "https",
},
{
desc: "Endpoint overrides emulator host when both are specified - http",
CustomEndpoint: "http://fake.gcs.com:8080/storage/v1",
StorageEmulatorHost: "https://emu.com",
WantRawBasePath: "http://fake.gcs.com:8080/storage/v1",
WantReadHost: "fake.gcs.com:8080",
WantScheme: "http",
},
}
Expand All @@ -1299,13 +1320,13 @@ func TestWithEndpoint(t *testing.T) {
}

if c.raw.BasePath != tc.WantRawBasePath {
t.Errorf("raw.BasePath not set correctly: got %v, want %v", c.raw.BasePath, tc.WantRawBasePath)
t.Errorf("%s: raw.BasePath not set correctly\n\tgot %v, want %v", tc.desc, c.raw.BasePath, tc.WantRawBasePath)
}
if c.readHost != tc.WantReadHost {
t.Errorf("readHost not set correctly: got %v, want %v", c.readHost, tc.WantReadHost)
t.Errorf("%s: readHost not set correctly\n\tgot %v, want %v", tc.desc, c.readHost, tc.WantReadHost)
}
if c.scheme != tc.WantScheme {
t.Errorf("scheme not set correctly: got %v, want %v", c.scheme, tc.WantScheme)
t.Errorf("%s: scheme not set correctly\n\tgot %v, want %v", tc.desc, c.scheme, tc.WantScheme)
}
}
os.Setenv("STORAGE_EMULATOR_HOST", originalStorageEmulatorHost)
Expand Down