impl(gax-internal): add universe_domain mod helper #5243
impl(gax-internal): add universe_domain mod helper #5243alvarowolfx merged 3 commits intogoogleapis:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5243 +/- ##
=======================================
Coverage 98.02% 98.02%
=======================================
Files 214 215 +1
Lines 44645 44706 +61
=======================================
+ Hits 43764 43824 +60
- Misses 881 882 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dbolduc
left a comment
dbolduc
left a comment
There was a problem hiding this comment.
I think you should split the test. And then I had a question on what behavior we want for one of the cases.
| .unwrap_or(DEFAULT_UNIVERSE_DOMAIN) | ||
| .to_string(); | ||
|
|
||
| let cred_universe = cred_universe.as_deref().unwrap_or(DEFAULT_UNIVERSE_DOMAIN); |
There was a problem hiding this comment.
So if my creds have no universe, and my client has a non-GDU universe, that is a fail. Should it be?
(There is not a test case for this)
There was a problem hiding this comment.
Yes, it should be a fail. No universe means GDU. If the client provides a non GDU, it's mismatch. I've added a test case.
There was a problem hiding this comment.
No universe means GDU
This implies to me that we messed up the Credentials::universe_domain() API. It should return a String instead of an Option<String>... oh well, too late to fix.
alvarowolfx
left a comment
alvarowolfx
left a comment
There was a problem hiding this comment.
I pushed a commit addressing review comments and also answered the question around the credential having no universe domain (which mean it's GDU)
| .unwrap_or(DEFAULT_UNIVERSE_DOMAIN) | ||
| .to_string(); | ||
|
|
||
| let cred_universe = cred_universe.as_deref().unwrap_or(DEFAULT_UNIVERSE_DOMAIN); |
There was a problem hiding this comment.
Yes, it should be a fail. No universe means GDU. If the client provides a non GDU, it's mismatch. I've added a test case.
| let cred = mock_credentials(cred_domain); | ||
|
|
||
| let universe_domain = resolve(client_override, &cred).await?; | ||
| assert_eq!(universe_domain.as_str(), expected, "{universe_domain:?}"); |
There was a problem hiding this comment.
nit:
| assert_eq!(universe_domain.as_str(), expected, "{universe_domain:?}"); | |
| assert_eq!(universe_domain, expected); |
| .unwrap_or(DEFAULT_UNIVERSE_DOMAIN) | ||
| .to_string(); | ||
|
|
||
| let cred_universe = cred_universe.as_deref().unwrap_or(DEFAULT_UNIVERSE_DOMAIN); |
There was a problem hiding this comment.
No universe means GDU
This implies to me that we messed up the Credentials::universe_domain() API. It should return a String instead of an Option<String>... oh well, too late to fix.
2 participants
Towards #3646