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

variable STORAGE_EMULATOR_HOST #202

Closed
abuckenheimer opened this issue Sep 4, 2020 · 3 comments · Fixed by #557
Closed

variable STORAGE_EMULATOR_HOST #202

abuckenheimer opened this issue Sep 4, 2020 · 3 comments · Fixed by #557

Comments

@abuckenheimer
Copy link

abuckenheimer commented Sep 4, 2020

Hey folks,

I'm looking into how to use https://github.com/fsouza/fake-gcs-server to test both googles standard storage client google.cloud.storage.Storage and the gcloud.aio.storage.Storage client at the same time. google.cloud.storage.Storage just made it possible to pass client_options=ClientOptions(api_endpoint='https://your_storage_emulator_address') to the client at initialization time while the glcoud-aio relies on a module wide global. Would you be open to adding a similar initialization option for the gcloud.aio.storage.Storage client? The module wide global is problematic because there's no way to monkeypatch it safely in an async context which means you can only talk to one API_ENDPOINT at a time. Would be happy to help with implementation on this if this sounds reasonable.

Thanks in advance!

@TheKevJames
Copy link
Member

Ah, interesting, looks like Google has been changing their approach here (parent ticket: googleapis/google-cloud-python#8475) -- used to be the case that each of their systems was configured to inter-operate with the CLI tool's env-init methods to support this.

Yeah, we could get behind updating our support here! I guess the most reasonable thing would be for all the gcloud-aio-* packages to support both methods? Perhaps have the constructor args override the env vars, which in turn override the defaults? Definitely open to your thoughts here, if you have any suggestions :)

We'd definitely accept a PR to help us move in this direction, though ideally we'd come up with a reasonable enough approach that can be applied to all the gcloud-aio-* packages, so if you're only interested in getting this set up for Storage please let me know so I can extend your PR for supporting more use-cases!

@josipbudzaki
Copy link

josipbudzaki commented Nov 2, 2022

I think this PR can be applied to other packages as well.
There is a similar PR for another package.
At first I wanted to solve it similarly, but noticed that others might have patched it e.g. in their unit tests.
So I opted for the "not-so-beautiful" but most backwards compatible solution.

However, the maintainers should discuss and decide which approach to implement across all packages.

TheKevJames added a commit that referenced this issue Dec 22, 2022
Solves a couple related goals in a consistent way across all projects:

* allows users to provide an api host via code instead of only via env
  var
* load that value at time of constructing the client rather than import
  time

Thanks to @josipbudzaki in #542 and @adriangb in #539 for providing a
baseline approach.

Fixes #202.
@TheKevJames
Copy link
Member

I've gone ahead and created #557, merging together the work from various folks in various per-project PRs (thanks @josipbudzaki!) along with our thoughts on having a single approach for all projects. Hopefully, this shoud solve both the problems of letting folks set these host values at time of object initialization as well as via code instead of env var.

My plan is to merge and release that code shortly, just waiting on review!

TheKevJames added a commit that referenced this issue Dec 22, 2022
Solves a couple related goals in a consistent way across all projects:

* allows users to provide an api host via code instead of only via env
  var
* load that value at time of constructing the client rather than import
  time

Thanks to @josipbudzaki in #542 and @adriangb in #539 for providing a
baseline approach.

Fixes #202.
TheKevJames added a commit that referenced this issue Dec 22, 2022
Solves a couple related goals in a consistent way across all projects:

* allows users to provide an api host via code instead of only via env
  var
* load that value at time of constructing the client rather than import
  time

Thanks to @josipbudzaki in #542 and @adriangb in #539 for providing a
baseline approach.

Fixes #202.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants