-
Notifications
You must be signed in to change notification settings - Fork 232
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: multi universe support, adding universe_domain field #1282
Conversation
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.
I didn't comment on all of them, but lets be consistent with our java docs (capitalization, periods, etc)
Co-authored-by: Leo <39062083+lsirac@users.noreply.github.com>
Kudos, SonarCloud Quality Gate passed! |
Agree for the javadocs body, except single sentence @params and other keywords.. its is mostly lowcaps, no period now. |
Co-authored-by: Leo <39062083+lsirac@users.noreply.github.com>
Co-authored-by: Leo <39062083+lsirac@users.noreply.github.com>
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.
feel free to ignore nits; noticing that use of explicit "this." is pretty common and comes down to personal preference :)
@@ -54,6 +56,19 @@ public abstract class Credentials implements Serializable { | |||
*/ | |||
public abstract String getAuthenticationType(); | |||
|
|||
/** | |||
* Returns the universe domain for the credential. |
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.
Similar to other APIs this should indicate that the call is blocking, and may refresh tokens as necessary to align them to the universe domain.
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.
Added, thanks!
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.
Not sure I understand why we mention token refreshes or that is it blocking? Seems reasonable that all calls are blocking unless the API specifies otherwise.
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
@@ -71,7 +77,10 @@ public class GoogleCredentials extends OAuth2Credentials implements QuotaProject | |||
* @return the credentials instance | |||
*/ | |||
public static GoogleCredentials create(AccessToken accessToken) { | |||
return GoogleCredentials.newBuilder().setAccessToken(accessToken).build(); | |||
return GoogleCredentials.newBuilder() |
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.
deprecate this constructor in favor of the builder?
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.
Done
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.
I think you forgot to deprecate it?
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.
Just left a couple more nits to address, thank you.
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
oauth2_http/java/com/google/auth/oauth2/ExternalAccountCredentials.java
Outdated
Show resolved
Hide resolved
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
This adds a multi-universe support to all the credentials.
Also adds reading a universe_domain field from ServiceAccount and modifies it for ExternalAccounts
A related change is to manage (builder, toString/hashCode/equals) common fields: universeDomain and quotaProjectId in the base GoogleCredentials class so we don't manage them in all the Credentials like before.