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

Core: Move google-cloud-core functionality into api_core #2

Open
1 of 8 tasks
theacodes opened this issue Oct 26, 2017 · 7 comments
Open
1 of 8 tasks

Core: Move google-cloud-core functionality into api_core #2

theacodes opened this issue Oct 26, 2017 · 7 comments
Labels
type: cleanup An internal cleanup or hygiene concern.

Comments

@theacodes
Copy link

@theacodes theacodes self-assigned this Oct 26, 2017
@theacodes
Copy link
Author

theacodes commented Oct 26, 2017

Scattered thoughts on stuff for my own future benefit (feel free to comment, but just know these are raw).

Clients:

  1. We should just drop the common base class and depend on google.auth.default() for both default credential and project ID detection.
  2. We should add a mixin class for from_service_account_file() and from_service_account_info(), with a deprecated alias for from_service_account_json. This can be used by gapic clients as well.

Connection/JSONConnection:

  1. Stop using inheritance for configuration.
  2. We should try to just treat this the same we do gRPC channels in gapic clients. The clients should accept a connection argument and construct their own if not provided.
  3. Connections should be reduced to just a basic class with an http property and a request method and made public.
  4. The request method should handle serialization, deserialization, exceptions, and injecting clientinfo headers.
  5. Storage seems to be the only client that does special things with the connection, so the bulk of the existing layers of code should be moved into storage. It's special enough to warrant that.
  6. Some clients write "Stub"-like classes (like _HTTPVisionAPI). This is fine and aligns with the plans above.

@dhermes
Copy link
Contributor

dhermes commented Oct 26, 2017

@jonparrott I actually massively reduced the interface of Connection with plans to remove it (and I made it a non-public member intentionally). So I am all for it!

@chemelnucfin chemelnucfin changed the title Move google-cloud-core functionality into api_core Core: Move google-cloud-core functionality into api_core Nov 3, 2017
@theacodes
Copy link
Author

Okay, here's my proposal for removing google.cloud.core.client and somewhat addressing #4840.

First, we will no longer have base classes for providing the credentials,
project, and _http properties. Each client will have to implement this.
We could do an abstract base class, but I feel it's overkill (I can be
convinced otherwise).

We'll add an api_core helper for getting credentials and a project ID.

class SomeClient:
    SCOPES = [...]

    def __init__(self, credentials=None):
        self.credentials, _ = (
            google.api_core.client_helpers.get_default_credentials_and_project(
                credentials, scopes=self.SCOPES))

    @property
    def credentials():
        return self._credentials

Some clients need/use project id:

class SomeClient:
    def __init__(self, credentials=None, project=None):
        self._credentials, self._project = (
            google.api_core.client_helpers.get_default_credentials_and_project(
                credentials,
                project,
                scopes=self.SCOPES,
                require_project=True))

    @property
    def credentials():
        return self._credentials

    @property
    def project():
        return self._project

Some clients need to construct an HTTP object, underlying gapic, etc.

# HTTP
class SomeClient:
    def __init__(self, credentials=None, project=None, _http=None):
        self._credentials, self._project = (
            google.api_core.client_helpers.get_default_credentials_and_project(
                credentials,
                project,
                scopes=self.SCOPES,
                require_project=True))
        self._http = google.api_core.http_helpers.make_authorized_session(
            self._credentials)


# GAPIC is easy
class SomeClient:
    def __init__(self, credentials=None, project=None, _channel=None):
        self._credentials, self._project = (
            google.api_core.client_helpers.get_default_credentials_and_project(
                credentials,
                project,
                scopes=self.SCOPES,
                require_project=True))
        self._gapic = some_api_v1.SomeClient(
            credentials=self._credentials, channel=_channel)

Now let's get tricky: some clients need to support emulators.

class SomeClient:
    EMULATOR_ENV_VAR = '...'
    EMULATOR_PROJECT_ENV_VAR = '...'  # optional

    def __init__(self, credentials=None, project=None, _channel=None):
        emulator_address = os.environ.get(EMULATOR_ENV_VAR)

        if emulator_address:
            self._credentials = google.auth.AnonymousCredentials()
            # Optional
            self._project = os.environ.get(
                EMULATOR_PROJECT_ENV_VAR, 'project-id')
        else:
            self._credentials, self._project = (
                google.api_core.client_helpers.get_default_credentials_and_project(
                    credentials, scopes=self.SCOPES))

In addition to just that, though, grpc clients need to detect this case and
create a non-secute channel to the emulator

        if emulator_address:
            channel = google.api_core.grpc_helper.make_emulator_channel(
                emulator_address)

        ...

        self._gapic = some_api_v1.SomeClient(
            credentials=self._credentials, channel=_channel)

That may seem like a lot of code, but doing away with the inheritance based stuff for credentials/project/http will actually simplify a lot of code (see Datastore). This can also be done in a completely non-breaking way (yay).

@lukesneeringer @tseaver WDYT?

@tseaver
Copy link
Contributor

tseaver commented Mar 9, 2018

I'm fine with the notion overall.

One note: the "construct an HTTP object" bit needs to account for the possibility that the user passed in _http.

@theacodes
Copy link
Author

One note: the "construct an HTTP object" bit needs to account for the possibility that the user passed in _http.

Good catch. Anything else I might have missed?

@theacodes
Copy link
Author

Necroing this bug to bring it to the attention of @crwilcox - this is what's needed to kill google-cloud-core.

@busunkim96 busunkim96 transferred this issue from googleapis/google-cloud-python Feb 7, 2020
@busunkim96 busunkim96 added the type: cleanup An internal cleanup or hygiene concern. label Feb 7, 2020
@msuozzo
Copy link

msuozzo commented Dec 14, 2021

Any plans to make this happen?

Also, anecdotally, exceptions seem to be the most common direct dependency on this package. I'd be curious as to how those symbols get migrated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

No branches or pull requests

6 participants