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

Adding optional switch to capture project ID in from_service_account_json(). #3436

Merged
merged 1 commit into from
Jun 6, 2017

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