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

Re-enable 'Bucket.requester_pays' feature #4056

Merged
merged 17 commits into from
Oct 12, 2017

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Sep 25, 2017

Closes #3474.

Needs extra system tests which pass user_project to the various APIs which take it, in the context of a bucket which is created with requester_pays enabled.

@tseaver tseaver added api: storage Issues related to the Cloud Storage API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Sep 25, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 25, 2017
@dhermes
Copy link
Contributor

dhermes commented Sep 25, 2017

@tseaver How much of this has been reviewed and how much is new? (Sorry if that is a question I should know the answer to.)

@tseaver
Copy link
Contributor Author

tseaver commented Sep 25, 2017

@dhermes This was all reviewed back in June: I just re-built the branch because the reversion made it hard to see what would be landed this time.

@dhermes
Copy link
Contributor

dhermes commented Sep 25, 2017

@tseaver Great. So no review needed?

@tseaver
Copy link
Contributor Author

tseaver commented Sep 25, 2017

@dhermes Not until we resolve whether / how we will implement the additional system tests.

@tseaver
Copy link
Contributor Author

tseaver commented Oct 5, 2017

@lukesneeringer I believe that this branch is ready to merge to master. Two open issues:

@tseaver tseaver removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 5, 2017
@tseaver
Copy link
Contributor Author

tseaver commented Oct 5, 2017

Unrelated AppVeyor failure (see #4128).

@tseaver
Copy link
Contributor Author

tseaver commented Oct 9, 2017

@lukesneeringer AFAIK, this branch is ready to merge.

Also, add 'requester_pays' argument to 'Client.create_bucket'.

Add a system test which exercises the feature.

Note that the new system test is skipped, because 'Buckets.insert' fails
with the 'billing/requesterPays' field set, both in our system tests and
in the 'Try It!' form in the docs.

Toward #3474.
* Add abstract '_PropertyMixin.user_project' property.

* Support 'user_project' in '_PropertyMixin.{reload,patch}'.

* Add 'user_project' param to 'Bucket.__init__'.

* Save and expose via read-only 'user_project' property.

* Implement 'Blob.user_property' via bucket's value.
* Block 'Bucket.create' if 'user_project' set:  the API does not accept that parameter.
Back-end used to reject it, but now allows it.
…ct' set (#4084)

* Pass through extra posargs for system tests.

* Plumb 'user_project' arg through 'Client.bucket'.
@tseaver tseaver force-pushed the storage-requester_pays-feature branch from 41fca6f to b0e1c96 Compare October 10, 2017 21:19
@tseaver tseaver merged commit aeb4cd4 into master Oct 12, 2017
@tseaver tseaver deleted the storage-requester_pays-feature branch October 12, 2017 14:43
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. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants