Skip to content

test: auth change CI recognition#12846

Draft
blakeli0 wants to merge 2 commits intomainfrom
test-auth-change-ci
Draft

test: auth change CI recognition#12846
blakeli0 wants to merge 2 commits intomainfrom
test-auth-change-ci

Conversation

@blakeli0
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +198 to +201
/** Dummy method to test CI recognition of auth changes. */
public String getTestString() {
return "test";
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Comment on lines +732 to +734
if (credentials != null) {
credentials.getTestString();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant