Skip to content

Conversation

@dhermes
Copy link
Contributor

@dhermes dhermes commented May 17, 2017

Fixes #1883.

@lukesneeringer This is a long-standing bug but should really make it into a google-cloud-core release. I know you hate releasing core.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 17, 2017
@theacodes
Copy link
Contributor

theacodes commented May 17, 2017

It would be great to solve this by just consolidating ClientWithProject and ClientWithCredentials Client

@dhermes
Copy link
Contributor Author

dhermes commented May 17, 2017

@jonparrott See the table I made about usage. The only issue with that is some APIs don't use a project (and we don't want to fail their constructors over a missing project).

@theacodes
Copy link
Contributor

theacodes commented May 17, 2017 via email

@dhermes
Copy link
Contributor Author

dhermes commented May 17, 2017

@jonparrott Mostly it doesn't help because it isn't true? :trollface:

Or do you mean something else?

@theacodes
Copy link
Contributor

theacodes commented May 17, 2017 via email

@dhermes
Copy link
Contributor Author

dhermes commented May 17, 2017

But there is no google.auth.default_project?

@theacodes
Copy link
Contributor

theacodes commented May 17, 2017 via email

@dhermes
Copy link
Contributor Author

dhermes commented May 17, 2017

Makes sense.

@lukesneeringer WDYT of us just using project on every Client instance? (This would just bring Client and ClientWithProject together, ClientWithProject is already a tiny impl.)

@tseaver
Copy link
Contributor

tseaver commented May 18, 2017

FTR, ClientWithProject is skipped by the following:

$ for f in $(ls */google/cloud/*/client.py); do
       if ! grep -q ClientWithProject $f; then
           echo $f
       fi
  done
bigtable/google/cloud/bigtable/client.py
language/google/cloud/language/client.py
resource_manager/google/cloud/resource_manager/client.py
spanner/google/cloud/spanner/client.py
speech/google/cloud/speech/client.py
translate/google/cloud/translate/client.py

Of those, the following have _ClientProjectMixin included:

$ for f in $(ls */google/cloud/*/client.py); do
       if ! grep -q ClientWithProject $f; then
           echo $f
       fi
  done | xargs grep -l _ClientProjectMixin
bigtable/google/cloud/bigtable/client.py
spanner/google/cloud/spanner/client.py

@dhermes
Copy link
Contributor Author

dhermes commented May 19, 2017

@tseaver This is (intended to be) captured in my table

# Check that mocks were called as expected.
file_open.assert_called_once_with(
mock.sentinel.filename, 'r', encoding='utf-8')
constructor.assert_called_once_with(info)

This comment was marked as spam.

This comment was marked as spam.

@dhermes dhermes merged commit a55010f into googleapis:master Jun 6, 2017
@dhermes dhermes deleted the fix-1883 branch June 6, 2017 17:12
@dhermes
Copy link
Contributor Author

dhermes commented Jun 6, 2017

@tseaver Sorry I missed your LGTM 18 days ago.

It's good I got this in, in case we have a google-cloud-core release for your gRPC remap PR, this can go in too.

landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 21, 2017
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
landrito pushed a commit to landrito/google-cloud-python that referenced this pull request Aug 22, 2017
parthea pushed a commit that referenced this pull request Oct 21, 2023
…rm/python-docs-samples#3436)

* [container_registry] fix: fix broken test

fixes #3435

* Use Pub/Sub message receiver that can notify main thread
  when it has received expected number of messages.
* Only test one single occurence.
* Use uuid4 wherever makes sense.
* test if Pub/Sub client receives at least one message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: core cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants