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

Decouple connection from Blob #823

Merged
merged 5 commits into from
Apr 16, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Apr 14, 2015

NOTE: Has #822 as diffbase.

Towards #728

@tseaver I made a separate commit for each method with used Blob.connection but can combine them if you like.

I wanted to get rid of the Blob.connection property, but _PropertyMixin.reload() and _PropertyMixin.patch() use it as well as ObjectACL.reload() and ObjectACL.save().

@dhermes dhermes added the api: storage Issues related to the Cloud Storage API. label Apr 14, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 14, 2015
@dhermes dhermes changed the title Decouple connection in blob Decouple connection from Blob Apr 14, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5cf9023 on dhermes:decouple-connection-in-blob into 6283ab9 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Apr 15, 2015

9932f0d LGTM
e429f9a LGTM
8ebb280 see comments inline (setting connection=None in ctors to verify that override is used)
824ff20 same issue
5cf9023 same issue

@dhermes dhermes force-pushed the decouple-connection-in-blob branch from 5cf9023 to a54b630 Compare April 15, 2015 21:50
@dhermes
Copy link
Contributor Author

dhermes commented Apr 15, 2015

@tseaver I imagine this being a transient issue since at some point connection will not be bound to any of the types or to the stubs.

I'm not sure what would need to be checked either.

@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

@dhermes in the tests, we should pass None to the bucket ctor in order to ensure that the passed-in connection is being used (the tests as we have them would pass if the methods took the connection argument, but then fell back to using self.connection).

@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

The heisenbug that @jgeewax fixed in #834 raised it's head here too. Only the 2nd time I had seen (build failure was first). Will rebase.


@tseaver All the Blob methods I changed use the passed in connection only and don't fall back to self.connection.

They are moving towards no connection anywhere, so falling back is a no-no.

@dhermes dhermes force-pushed the decouple-connection-in-blob branch from a54b630 to ca34a4a Compare April 16, 2015 16:30
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ca34a4a on dhermes:decouple-connection-in-blob into 40d84e9 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

That's why having the bucket initialized with an invalid connection is the Right Thing (TM): it ensures that fallback cannot cause the test to succeed.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

Gotcher. Current plan is to cherry-pick each of the 5 commits and then replace Bucket(connection) with Bucket(None) or similar. (About to spend an hour without internet on BART.)

LMK if this is a bad idea.

This is to blob and bucket can use it without import cycles.
@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

SGTM

Allowing the default connection to be used or direct use of
a credentials object.
Allowing the default connection to be used.
Allowing the default connection to be used as fallback or takes
an explicit connection argument.
Allowing the default connection to be used as fallback or takes
an explicit connection argument.
@dhermes dhermes force-pushed the decouple-connection-in-blob branch from ca34a4a to 60d43ac Compare April 16, 2015 18:24
@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

@tseaver PTAL

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 60d43ac on dhermes:decouple-connection-in-blob into 40d84e9 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

LGTM

dhermes added a commit that referenced this pull request Apr 16, 2015
@dhermes dhermes merged commit 06e26ad into googleapis:master Apr 16, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

@tseaver Can we divvy up work and try to close out #728 together?

@dhermes dhermes deleted the decouple-connection-in-blob branch April 16, 2015 19:31
@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

How do you see the split?

@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

Can you handle the methods in acl.py and I'll try to cover the rest (Bucket and _PropertyMixin)?

Then we "converge" and remove the connection property from Blob (it gets used in acl.py).

@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

OK. I just inventoried those methods in #825.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

Awesome! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants