[GitHub] [tinkerpop] Cole-Greer commented on a diff in pull request #1989: TINKERPOP-2747 Separate AuthInfo struct vs AuthInfoProvider interface for dynamic credentials

["Cole-Greer (via GitHub)" <gi...@apache.org>]

Cole-Greer commented on code in PR #1989:
URL: https://github.com/apache/tinkerpop/pull/1989#discussion_r1134797403


##########
gremlin-go/driver/connection.go:
##########
@@ -42,15 +42,15 @@ type connection struct {
 }
 
 type connectionSettings struct {
-	authInfo         			*AuthInfo
-	tlsConfig         			*tls.Config
-	keepAliveInterval 			time.Duration
-	writeDeadline     			time.Duration
-	connectionTimeout 			time.Duration
-	enableCompression 			bool
-	readBufferSize				int
-	writeBufferSize				int
-	enableUserAgentOnConnect		bool
+	authInfo                 AuthInfoProvider

Review Comment:
   I do have some concerns with potentially breaking things for some users by changing this type from `*AuthInfo` to `AuthInfoProvider`. As the fields in AuthInfo were all public, there are likely users who are modifying these fields directly instead of using the interface.
   
   It might not be a big deal as the current expected use case is users construct the AuthInfo struct, pass it into a new client, then never touch it again. I don't believe the AuthInfo is publicly accessible anywhere after it has been passed into the client.
   
   I want to think about this a little bit more but I believe it is ok and won't impact any users.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org