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

Unify '_implicit_environ' handling #741

Closed
tseaver opened this issue Mar 17, 2015 · 6 comments
Closed

Unify '_implicit_environ' handling #741

tseaver opened this issue Mar 17, 2015 · 6 comments
Assignees
Labels
api: datastore Issues related to the Datastore API. api: pubsub Issues related to the Pub/Sub API. api: storage Issues related to the Cloud Storage API.

Comments

@tseaver
Copy link
Contributor

tseaver commented Mar 17, 2015

There is a lot of overlap between datastore._implicit_environ and storage._implicit_environ. We should factor that out, especially before implementing yet another version in pubsub.

@tseaver tseaver added api: datastore Issues related to the Datastore API. api: storage Issues related to the Cloud Storage API. hygiene api: pubsub Issues related to the Pub/Sub API. labels Mar 17, 2015
@dhermes
Copy link
Contributor

dhermes commented Mar 18, 2015

I like it. Was trying to move gradually towards this while building out the implicit functionality into storage.

The hardest thing to factor out is the dependence on the local concepts of Connection and _DefaultsContainer.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 20, 2015

Here's my notional take on a unification plan;

  • Move the GAE/GCD detection logic from gcloud.datastore._implicit_environ into gcloud._helpers (#741: hoist GAE/GCD detection into gcloud._helpers #747).
  • Move the "project detection" logic from gcloud.storage._implicit_environ into gcloud._helpers (#741: hoist project detection/setup into 'gcloud._helpers'. #748).
  • Consider how 'project' and 'dataset_id' should be related (e.g., does an app running on AppEngine / GCE use the same logic to determine its project ID for storage / pubsub purposes?).
  • Add gcloud._helpers.get_scoped_connection(), which takes the connection class and a scopes list as arguments; make each gcloud.*._implicit_environ.get_connection delegate to that helper (e.g., via functools.partial) (#741: hoist get_scoped_connection into connection #749 leaves to original get_connection() functions in place, but has them delegate to get_scoped_connection()).
  • Move the _DefaultsContainer up into gcloud._helpers, and make it generic; instantiate it (subclassing only if really necessary) in each gcloud.*._implicit_environ.

@dhermes WDYT?

@dhermes
Copy link
Contributor

dhermes commented Mar 20, 2015

I don't see _DefaultsContainer able to be generalized due to the dependence on package specific functions for lazy-loading.

The rest seems great.

RE: Project ID vs. App Engine App ID. We should ask someone with more knowledge of the current state of things. I think they are the same, but could be wrong.

App Engine apps also have a default bucket created for them: <app-id>.appspot.com (bucket names with periods can't usually be created)

tseaver added a commit that referenced this issue Mar 20, 2015
#741: hoist GAE/GCD detection into `gcloud._helpers`
@tseaver
Copy link
Contributor Author

tseaver commented Mar 20, 2015

Re: _DefaultsContainer: I (now) agree. When I moved the project stuff up into gcloud._helpers, I split apart the _DefaultsContainer class in gcloud.storage._implicit_environ: each modules version knows only about the attributes relevant to its own functions.

tseaver added a commit that referenced this issue Mar 23, 2015
#741: hoist project detection/setup into 'gcloud._helpers'.
tseaver added a commit that referenced this issue Mar 24, 2015
#741: hoist `get_scoped_connection` into `connection`
@dhermes
Copy link
Contributor

dhermes commented Mar 31, 2015

@tseaver What's the status on this?

@tseaver
Copy link
Contributor Author

tseaver commented Mar 31, 2015

I think that we are good, here: the default project stuff is shared, as is the repeated connection stuff (via get_scoped_connection).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. api: pubsub Issues related to the Pub/Sub API. api: storage Issues related to the Cloud Storage API.
Projects
None yet
Development

No branches or pull requests

2 participants