-
Notifications
You must be signed in to change notification settings - Fork 90
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: add common logic for supporting universe domain #621
Conversation
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
…on-api-core into add-helpers-for-universe
…on-api-core into add-helpers-for-universe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, but I left some comments.
One other comment: do you want to call this module universe_helpers
?
from google.api_core import universe_helpers
universe_helpers.get_universe_domain()
universe_helpers.compare_universes()
I wonder whether there are better names for this module and methods.
Maybe universe_domain.determine()
and universe_domain.compare()
? Or maybe we need slightly more explicit names than that (universe.determine_domain()
, universe.compare_domains()
?)
(Feel free to push back)
…on-api-core into add-helpers-for-universe
…on-api-core into add-helpers-for-universe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you either use or remove the mTLS_Universe_Error
declaration from this PR?. Right now it's not being used anywhere.
Aside from that, just a minor (non-blocking) testing suggestion.
assert str(excinfo.value).find(fake_domain) >= 0 | ||
|
||
with pytest.raises(universe.UniverseMismatchError) as excinfo: | ||
universe.compare_domains(fake_domain, _Fake_Credentials()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These won't fit in one line? Darn.
Fixes #624 🦕