Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a dummy method getTestString to the Credentials class and invokes it within GrpcCallContext.validateUniverseDomain to test CI recognition. The review feedback highlights that adding a public method to a core class modifies the public API unnecessarily and suggests using annotations like @InternalApi or @VisibleForTesting. Furthermore, the invocation in GrpcCallContext is flagged as dead code because the return value is not utilized.
| /** Dummy method to test CI recognition of auth changes. */ | ||
| public String getTestString() { | ||
| return "test"; | ||
| } |
There was a problem hiding this comment.
Adding a public method to the Credentials class modifies the library's public API. For a core class, introducing "dummy" methods can lead to maintenance issues and confusion for external consumers. If this method is required for cross-repository CI testing, it should be restricted using an annotation like @InternalApi or @VisibleForTesting to prevent it from being considered part of the stable public API.
| if (credentials != null) { | ||
| credentials.getTestString(); | ||
| } |
There was a problem hiding this comment.
The call to credentials.getTestString() is a no-op as the return value is not utilized. This introduces dead code into the validateUniverseDomain method. While this is a test PR, such code should not be present in the final implementation to avoid unnecessary execution overhead and maintain code clarity.
This is a draft PR to test if changes in google-auth-library-java are correctly recognized by CI in sdk-platform-java. It adds a dummy method to Credentials and uses it in GrpcCallContext.