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

feat: allow credentials files to be passed for channel creation #50

Merged
merged 16 commits into from Jun 18, 2020

Conversation

busunkim96
Copy link
Contributor

@busunkim96 busunkim96 commented Jun 15, 2020

Allow credentials files to be passed to create_channel.

For googleapis/gapic-generator-python#432

While it's possible for the library transport to create a credentials object and pass it to create_channel, I think it will be easier in the long run for the auth logic to remain in _create_composite_credentials.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 15, 2020
@busunkim96 busunkim96 changed the title feat: let create_channel accept credentials files feat: allow credential files to be passed for channel creation Jun 15, 2020
@@ -218,7 +227,7 @@ def _create_composite_credentials(credentials=None, scopes=None, ssl_credentials
)


def create_channel(target, credentials=None, scopes=None, ssl_credentials=None, **kwargs):
def create_channel(target, credentials=None, scopes=None, ssl_credentials=None, credentials_file=None, **kwargs):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added credentials_file to the end of the list to avoid breaking anyone who is using positional arguments.

@busunkim96 busunkim96 marked this pull request as ready for review June 16, 2020 23:37
@busunkim96 busunkim96 changed the title feat: allow credential files to be passed for channel creation feat: allow credentials files to be passed for channel creation Jun 16, 2020
Copy link
Contributor

@software-dov software-dov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, one style/preference concern with exception raising.

credentials=mock.sentinel.credentials
)

assert "mutually exclusive" in str(excinfo.value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of checking for explicit strings in exception messages. If it's easy and within the style guides of the repo, I like to make one-off exception types and check for the specific exception type within tests, something like

class DuplicateCredentialArgs(Exception):
   pass
...
with pytest.raises(DuplicateCredentialArgs) as exc:
   ...

@busunkim96 busunkim96 merged commit ded92d0 into master Jun 18, 2020
@busunkim96 busunkim96 deleted the creds-and-scope-overrides branch June 19, 2020 00:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants