Expose containers defaults - #2054

The main reason for this change is to reuse these defaults in the testcontainers-scala.

I have a bad feeling about testcontainers-scala and the approach of re-implementing the containers instead of just wrapping the java ones. Is it really necessary?

@bsideup well, actually, scala containers wrap java ones, not re-implement them :) In testcontainers-scala we wrap java containers to provide a more convenient, safe and familiar for the scala programmer way to construct containers. All runtime and construction logic is still on the java side.

Also, I have no idea why CI failed. It looks like this is not because of my changes — other branches are red too.

I'm also uncomfortable with this - it doesn't feel right that overall there should be two routes by which a default value could be passed in. The public API of the Java container classes should be the methods it exposes, not methods + a set of constants.

Please could we park this PR for now and have a discussion about how we can work together better to ensure that the Scala wrapper has less hard work like this to do? I think we'd all benefit from getting together to talk, particularly as we have some ideas for changing our APIs anyway - hopefully these ideas will make it easier, but we need to know if it's going to make it harder too.

Chat on Slack to arrange a video call some time?

@rnorth Not sure what you mean by "two routes by which a default value could be passed in". Could you explain that?

I see nothing dangerous in my change. Some of the containers already expose their defaults (more or less). I just made them all public. Why they should be private? There is nothing private in them. It's just a good practice to not hard-code things inside the method bodies and move them in the constants.

Even without testcontainers-scala argument, I think library users will benefit from this change. As I already said, it could be useful when you are creating custom containers on top of already created containers. I faced such scenarios a few times. This is not a very wide use case though.

Of course, we can discuss our long-term plans. But I afraid that all this can take a long time. I hoped that this pull request will be easily merged and I will continue to work on the testcontainers-scala.

Guys, 2 months have passed and I want to remind you about this pull request. These changes will help me a lot with this issue. Without these changes, I will have to copy-paste all defaults from the java library, which I really would not want to do.

@LMnet the PR is in "on hold" state because it does not feel right.

Even without testcontainers-scala argument, I think library users will benefit from this change. As I already said, it could be useful when you are creating custom containers on top of already created containers. I faced such scenarios a few times. This is not a very wide use case though.

It is indeed is not a very popular use case and so far I don't remember anyone to report the same problem.

I think what we should focus on is making testcontainers-scala use testcontainers-java definitions "as it is", not try to re-implement them.

We're working on bringing the container definitions to the Java version, so that there will be a separation of the configuration and started state.
I believe it will make Scala's life much easier and hope will lead to the removal of code duplicates in the scala wrapper (so that it actually becomes a wrapper).

I will ping you once the PR is ready so that you can take a look and give feedback whether it is useful for Scala or not. Sounds good?

FTR the definitions will most likely make it into 1.x as an alternative way of defining the containers and will be promoted as the only API in 2.x.

I very much doubt that it is possible to create such an API for containers on the Java side, which could be used as-is in scala without any kind of wrapper. Also, as I understand it, the new API in the Java library will not appear soon. But the changes that I propose will help me right now. Anyway, I (and I believe @dimafeng too) will give you feedback about the new Java API.

Also, I want to repeat myself: in testcontainers-scala we don't reimplement containers. In your example, almost all code in KafkaConainter is a construction code. It's a bit massive because we now have 2 versions of the API and we have to have some code duplication. But I want to pay attention — it's just a wrapper. It's not "reimplementation". All logic is on the java side.

I still don’t understand why this change met such resistance. This is a very simple change that does not break backward compatibility. It is not dangerous and does not add any potential problems, in my opinion. At the same time, it saves the project from one of the problems - hardcoded values. Even this, in my opinion, should be enough.

So, if the decision is final, I have to transfer the default values ​​to the scala side.

@LMnet

Also, as I understand it, the new API in the Java library will not appear soon.

I have a bit different information :)

I still don’t understand why this change met such resistance. This is a very simple change that does not break backward compatibility. It is not dangerous and does not add any potential problems, in my opinion.

It adds a lot of public information for us to maintain. Static nature of it makes it even harder, preventing us from changing things (we will have to keep it in sync, getters/parameters & their static values).
If you really need to get these defaults, why don't you use something like:

private static final String CASSANDRA_DEFAULT_USERNAME = new CassandraContainer().getUsername();

or similar (maybe lazy) constructions? So that you only use our public API which is always up to date with the implementation.

At the same time, it saves the project from one of the problems - hardcoded values.

We barely have any hardcoded values, most of them are configurable.

We barely have any hardcoded values, most of them are configurable.

Private string literals that used as default values are hardcode. Library users can't address these values, can't use them.

It adds a lot of public information for us to maintain. Static nature of it makes it even harder, preventing us from changing things (we will have to keep it in sync, getters/parameters & their static values).

static final things is absolutely not dangerous in terms of maintenance. It is just constants. And, because these constants are used as default values inside containers constructors, you don't need any extra effort to make them in-sync with getters/setters in the instantiated containers. The only possible problematic moment I can see is inconsistent naming. If someone decides to improve this he will have to do this through the deprecation process.

Getting default values from the instantiated containers is a hacky solution. Container creation could potentially contain some side-effectful logic. I don't want to instantiate things with side effects only to get some static constants, which are guaranteed never to change in the runtime. And also, not all containers can be instantiated with only default values. For example, you always need to provide an image name for the oracle container.

I don't want to instantiate things with side effects only to get some static constants

Since #2024 is merged, we no longer have side effects in our constructors.

Private string literals that used as default values are hardcode. Library users can't address these values, can't use them.

The users can get these values and set them to their preferred values in most of the places. Not sure what are you talking about.
Or maybe you're using the wrong term?

static final things is absolutely not dangerous in terms of maintenance. It is just constants. And, because these constants are used as default values inside containers constructors, you don't need any extra effort to make them in-sync with getters/setters in the instantiated containers.

That is your assumption. I would recommend trying some critical thinking to understand why your suggestion may not be as risk free as you think.

Due to our limited time capacity, given the workaround (getting default values from a dummy instance) and since container definitions are coming, I recommend we stop this conversation and focus on the right thing.

I do hope that eventually the Scala wrapper will become waaaaay thinner than it is right now and there won't be a need to get the defaults and re-implement containers' public interfaces.
/cc @dimafeng

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.

Bump

@LMnet unless you have new, more convincing arguments, I don't think there is anything more to discuss :)

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.

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.