You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2022/09/15 12:23:06 UTC
[GitHub] [shardingsphere] azexcy opened a new pull request, #21007: Add MySQL auth plugin case at migration IT
azexcy opened a new pull request, #21007:
URL: https://github.com/apache/shardingsphere/pull/21007
Changes proposed in this pull request:
- Add MySQL auth plugin case at migration IT
---
Before committing this PR, I'm sure that I have checked the following options:
- [x] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [x] I have self-reviewed the commit code.
- [x] I have triggered maven check: `mvn clean install -B -T2C -DskipTests -Dmaven.javadoc.skip=true -e`.
- [x] I have made corresponding changes to the documentation.
- [x] I have added corresponding unit tests for my changes.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz merged pull request #21007: Add MySQL 8 auth plugin case at migration IT
Posted by GitBox <gi...@apache.org>.
sandynz merged PR #21007:
URL: https://github.com/apache/shardingsphere/pull/21007
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] sandynz commented on a diff in pull request #21007: Add MySQL auth plugin case at migration IT
Posted by GitBox <gi...@apache.org>.
sandynz commented on code in PR #21007:
URL: https://github.com/apache/shardingsphere/pull/21007#discussion_r971943072
##########
shardingsphere-test/shardingsphere-integration-test/shardingsphere-integration-test-env/src/test/java/org/apache/shardingsphere/test/integration/env/container/atomic/storage/config/impl/StorageContainerConfigurationFactory.java:
##########
@@ -52,4 +54,29 @@ public static StorageContainerConfiguration newInstance(final DatabaseType datab
throw new RuntimeException(String.format("Database `%s` is unknown.", databaseType.getType()));
}
}
+
+ /**
+ * Create new instance of storage container configuration.
+ *
+ * @param databaseType database type
+ * @param command command
+ * @param containerEnvironments container environments
+ * @param mountedResources mounted resources
+ * @return created instance
+ */
+ public static StorageContainerConfiguration newInstance(final DatabaseType databaseType, final String command, final Map<String, String> containerEnvironments,
+ final Map<String, String> mountedResources) {
+ switch (databaseType.getType()) {
+ case "MySQL":
+ return MySQLContainerConfigurationFactory.newInstance(command, containerEnvironments, mountedResources);
+ case "PostgreSQL":
+ return PostgreSQLContainerConfigurationFactory.newInstance();
+ case "openGauss":
+ return OpenGaussContainerConfigurationFactory.newInstance();
+ case "H2":
+ return H2ContainerConfigurationFactory.newInstance();
+ default:
+ throw new RuntimeException(String.format("Database `%s` is unknown.", databaseType.getType()));
+ }
+ }
Review Comment:
It looks strange here, parameters are only used by MySQL type. Could we just create container configuration for MySQL directly? Use MySQLContainerConfigurationFactory
##########
shardingsphere-test/shardingsphere-integration-test/shardingsphere-integration-test-env/src/test/java/org/apache/shardingsphere/test/integration/env/container/atomic/storage/config/impl/mysql/MySQLContainerConfigurationFactory.java:
##########
@@ -35,25 +36,53 @@ public final class MySQLContainerConfigurationFactory {
/**
* Create new instance of MySQL container configuration.
- *
+ *
* @return created instance
*/
public static StorageContainerConfiguration newInstance() {
- return new StorageContainerConfiguration(getCommand(), getContainerEnvironments(), getMountedResources());
+ return new StorageContainerConfiguration(getDefaultCommand(), getDefaultContainerEnvironments(), getDefaultMountedResources());
}
- private static String getCommand() {
+ /**
+ * Create new instance of MySQL container configuration with parameter.
+ *
+ * @param command command
+ * @param containerEnvironments container environments
+ * @param mountedResources mounted resources
+ * @return created instance
+ */
+ public static StorageContainerConfiguration newInstance(final String command, final Map<String, String> containerEnvironments, final Map<String, String> mountedResources) {
+ return new StorageContainerConfiguration(ObjectUtils.defaultIfNull(command, getDefaultCommand()), ObjectUtils.defaultIfNull(containerEnvironments, getDefaultContainerEnvironments()),
+ ObjectUtils.defaultIfNull(mountedResources, getDefaultMountedResources()));
+ }
+
+ /**
+ * Get default command.
+ *
+ * @return command
+ */
+ public static String getDefaultCommand() {
return "--server-id=" + ContainerUtil.generateMySQLServerId();
}
- private static Map<String, String> getContainerEnvironments() {
+ /**
+ * Get default container environments.
+ *
+ * @return container environments
+ */
+ public static Map<String, String> getDefaultContainerEnvironments() {
Map<String, String> result = new HashMap<>(2, 1);
result.put("LANG", "C.UTF-8");
result.put("MYSQL_RANDOM_ROOT_PASSWORD", "yes");
return result;
}
- private static Map<String, String> getMountedResources() {
+ /**
+ * Get default mounted resources.
+ *
+ * @return container environments
+ */
+ public static Map<String, String> getDefaultMountedResources() {
Review Comment:
Looks it might be better to refactor MySQLContainerConfigurationFactory to MySQLContainerConfiguration, it could be easier to extend it. And also other databases impl.
But it need too much changes, so just keep it for now.
##########
shardingsphere-test/shardingsphere-integration-test/shardingsphere-integration-test-env/src/test/java/org/apache/shardingsphere/test/integration/env/container/atomic/storage/config/impl/mysql/MySQLContainerConfigurationFactory.java:
##########
@@ -35,25 +36,53 @@ public final class MySQLContainerConfigurationFactory {
/**
* Create new instance of MySQL container configuration.
- *
+ *
* @return created instance
*/
public static StorageContainerConfiguration newInstance() {
- return new StorageContainerConfiguration(getCommand(), getContainerEnvironments(), getMountedResources());
+ return new StorageContainerConfiguration(getDefaultCommand(), getDefaultContainerEnvironments(), getDefaultMountedResources());
}
- private static String getCommand() {
+ /**
+ * Create new instance of MySQL container configuration with parameter.
+ *
+ * @param command command
+ * @param containerEnvironments container environments
+ * @param mountedResources mounted resources
+ * @return created instance
+ */
+ public static StorageContainerConfiguration newInstance(final String command, final Map<String, String> containerEnvironments, final Map<String, String> mountedResources) {
+ return new StorageContainerConfiguration(ObjectUtils.defaultIfNull(command, getDefaultCommand()), ObjectUtils.defaultIfNull(containerEnvironments, getDefaultContainerEnvironments()),
+ ObjectUtils.defaultIfNull(mountedResources, getDefaultMountedResources()));
+ }
+
+ /**
+ * Get default command.
+ *
+ * @return command
+ */
+ public static String getDefaultCommand() {
return "--server-id=" + ContainerUtil.generateMySQLServerId();
}
- private static Map<String, String> getContainerEnvironments() {
+ /**
+ * Get default container environments.
+ *
+ * @return container environments
+ */
+ public static Map<String, String> getDefaultContainerEnvironments() {
Map<String, String> result = new HashMap<>(2, 1);
result.put("LANG", "C.UTF-8");
result.put("MYSQL_RANDOM_ROOT_PASSWORD", "yes");
return result;
}
- private static Map<String, String> getMountedResources() {
+ /**
+ * Get default mounted resources.
+ *
+ * @return container environments
+ */
+ public static Map<String, String> getDefaultMountedResources() {
Review Comment:
If these methods are just used in class itself, then it's not necessary to set as `public`
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org