imbajin commented on code in PR #724:
URL:
https://github.com/apache/hugegraph-toolchain/pull/724#discussion_r3079109690
##########
hugegraph-loader/assembly/travis/install-mysql.sh:
##########
@@ -23,12 +27,20 @@ TRAVIS_DIR=$(dirname $0)
CONF=hugegraph-test/src/main/resources/hugegraph.properties
MYSQL_USERNAME=root
Review Comment:
‼️ This skip check only matches an exact container name. GitHub Actions
service containers are usually not named `mysql`, so the guard will miss the
case you want to avoid and the script will still try to start a second MySQL
instance.
Consider checking whether port 3306 is already occupied or probing the
database itself instead.
##########
hugegraph-loader/src/main/java/org/apache/hugegraph/loader/source/jdbc/JDBCVendor.java:
##########
@@ -305,7 +305,8 @@ public String buildUrl(JDBCSource source) {
.setParameter("characterEncoding", Constants.CHARSET.name())
.setParameter("rewriteBatchedStatements", "true")
.setParameter("useServerPrepStmts", "false")
- .setParameter("autoReconnect", "true");
+ .setParameter("autoReconnect", "true")
Review Comment:
⚠️ `buildUrl()` is shared by MYSQL and POSTGRESQL. Putting a MySQL-only auth
workaround in the common path leaks this flag into the PostgreSQL URL too.
Please scope `allowPublicKeyRetrieval` to the MySQL branch instead.
```suggestion
uriBuilder.setParameter("useSSL", "false")
.setParameter("characterEncoding",
Constants.CHARSET.name())
.setParameter("rewriteBatchedStatements", "true")
.setParameter("useServerPrepStmts", "false")
.setParameter("autoReconnect", "true");
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
