enable database custom alias configuration #4 by SaschaJohn · Pull Request #2294 · testcontainers/testcontainers-java

SaschaJohn · 2020-01-27T09:40:49Z

@bsideup @rnorth
my approach so solve #4
I also added documenttaion and testcases.
Looking forward for feedback.

Regards

Sascha

bsideup · 2020-01-27T09:44:49Z

+import com.zaxxer.hikari.HikariDataSource;
+
+@RunWith(Parameterized.class)
+public class JDBCDriverAliasPropertiesTest {


This class should be a unit test. We should test that aliases map to the right db type/image, there is no need to actually start the DB

bsideup · 2020-01-27T09:46:40Z

+    	Properties p = new Properties();
+    	p.put("jdbc.alias.mysqlserver.type", "sqlserver");
+    	p.put("jdbc.alias.mysqlserver.image", "mcr.microsoft.com/mssql/server");
+    	ContainerDatabaseDriver.clearJBDCAliasProperties();


this should use the configuration mock, see other configuration-sensitive tests (e.g. tests for the reusable mode)

bsideup · 2020-01-27T09:47:02Z

+import com.zaxxer.hikari.HikariDataSource;
+
+@RunWith(Parameterized.class)
+public class JDBCDriverAliasTest {


should also be a unit test

bsideup · 2020-01-27T09:47:20Z

    }

-    private void performSimpleTest(HikariDataSource dataSource) throws SQLException {
+    protected void performSimpleTest(HikariDataSource dataSource) throws SQLException {


bsideup · 2020-01-27T09:47:59Z

@@ -1,2 +1,3 @@
 mcr.microsoft.com/mssql/server:2017-CU12
+mcmoe/mssqldocker:latest


please remove after converting into a unit test, we should not accept licenses of 3rd party images

bsideup · 2020-01-27T09:48:23Z

    private Driver delegate;
    private static final Map<String, Set<Connection>> containerConnections = new HashMap<>();
    private static final Map<String, JdbcDatabaseContainer> jdbcUrlContainerCache = new HashMap<>();
+    private static final Map<String, Map<String, String>> jdbcAliasMap = new HashMap<String, Map<String,String>>();


please make Map's value a type, not just Map<String, String>

bsideup · 2020-01-27T09:49:36Z

+     */
+    public ContainerDatabaseDriver() {
+    	super();
+    	mapJBDCAliasProperties(TestcontainersConfiguration.getInstance().getProperties());    	


Why cached? You can get the property when you encounter an alias

@bsideup we don't know the aliases without reading the configuration at instanciation.
IMHO it's the easiest way to implement it here, otherwise we won't be able to find candiate containers with the alias and have to see if the name is an alias afterwards...

but you don't need it at instantiation time.

The algorithm should be something like:

get the type from the JDBC URL

try to get alias by it

if not found, fallback to the default one (the behaviour before this PR)

bsideup · 2020-01-27T09:49:43Z

-
-            String queryString = connectionUrl.getQueryString().orElse("");
+        	
+        	String queryString = connectionUrl.getQueryString().orElse("");


please fix the formatting

bsideup · 2020-01-27T09:50:31Z

+     */
+    private String getAliasTag(Map<String, String> aliasMap, JdbcDatabaseContainer container) {
+    	String tag = aliasMap.get("tag");
+		if (tag == null) { // no tag was set, so use the default one


wrong formatting

bsideup · 2020-01-27T09:56:32Z

+                    	container = candidateContainerType.newInstance(connectionUrl);
+
+                    	if (wasAlias) { //overwrite image and tag
+                    		container.setDockerImageName(aliasMap.get("image")+":"+getAliasTag(aliasMap, container));                        	


This does not feel like the right place to do it. Consider handling aliases in JdbcDatabaseContainerProvider#newInstance. We may also need to introduce a new API there to handle new instance creation with custom image

@bsideup
newInstance() is overridden in implementing modules, I didn't want to touch existing modules.
For me it's the smartest way to overwrite the imageName right after instance creation.

this is why I was talking about introducing a new API. setDockerImageName here is a bit hacky and we should expose first-class support for creating JDBC containers from a custom image

@bsideup the Container is created according to the property DockerImageName. The property as a public setter isn't that an API ;-)

bsideup · 2020-01-27T09:58:18Z

+     * @param type		driver type of the alias specified e.g. "mysql", "sqlserver", ...
+     * @param image		string that represents the docker image e.g. "mysql", "customregistry/image name"
+     */
+    public static void registerAlias(String alias, String type, String image) {		


since we discourage the use of :latest, we should not provide any tag-less API

@bsideup
this will not fallback to latest but to the tag, that was specified by the module maintainer in the underlying specific container class.
The usecase here is when you host your own proxy registry, but want to use the tag, that was specified already. Somewhat related to feture request in #2100 at least for the db containers

bsideup

The implementation looks a bit over-engineered.
Also, it feels that a new API is required.

@SaschaJohn you can proceed if you feel confident, otherwise we can pick up your progress :)

SaschaJohn · 2020-01-31T15:26:29Z

@binkley, without specifying image tag the tag from the original implementing class is used,
for your example default the version, from the Postgres Container class.
With image tag, the code will use your explicit tag configuration. But you're right. Both examples in the doc are the same. I'll fix that

stale · 2020-04-30T16:33:54Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

rnorth · 2020-05-01T18:20:27Z

@bsideup I know you were grappling with a more difficult version of this problem for r2dbc support. Would you like to take over / replace this PR?

stale · 2020-08-01T08:59:07Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

stale · 2020-08-15T23:32:55Z

This issue has been automatically closed due to inactivity. We apologise if this is still an active problem for you, and would ask you to re-open the issue if this is the case.

Sascha John Hesse added 3 commits January 26, 2020 10:42

initial working version

bb16527

cleanup, testing

518b907

fixed tests, added documentation

7ffcaff

SaschaJohn requested review from bsideup, kiview and rnorth as code owners January 27, 2020 09:40

bsideup reviewed Jan 27, 2020

View reviewed changes

bsideup requested changes Jan 27, 2020

View reviewed changes

stale Bot added the stale label Apr 30, 2020

stale Bot removed the stale label May 1, 2020

stale Bot added the stale label Aug 1, 2020

stale Bot closed this Aug 15, 2020

		@@ -1,2 +1,3 @@
		mcr.microsoft.com/mssql/server:2017-CU12
		mcmoe/mssqldocker:latest


		String queryString = connectionUrl.getQueryString().orElse("");

		String queryString = connectionUrl.getQueryString().orElse("");

Uh oh!

Uh oh!

Conversation

SaschaJohn commented Jan 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsideup left a comment

Choose a reason for hiding this comment

Uh oh!

SaschaJohn commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale Bot commented Apr 30, 2020

Uh oh!

rnorth commented May 1, 2020

Uh oh!

stale Bot commented Aug 1, 2020

Uh oh!

stale Bot commented Aug 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SaschaJohn commented Jan 31, 2020 •

edited

Loading