Add alias for creating a generic container definition

[alejandrohdezma]

Just adds an alias for creating a ContainerDef for a GenericContainer that doesn't involve creating a new object/class.

Example:

import scala.io.Source

import java.net.URL

import org.scalatest.flatspec.AnyFlatSpec

import com.dimafeng.testcontainers.GenericContainer
import com.dimafeng.testcontainers.scalatest.TestContainerForAll

import org.testcontainers.containers.wait.strategy.Wait

class GenericContainerSpec extends AnyFlatSpec with TestContainerForAll {
  
  override val containerDef = GenericContainer.Def.anonymous("nginx:latest",
    exposedPorts = Seq(80),
    waitStrategy = Wait.forHttp("/")
  )

  "GenericContainer" should "start nginx and expose 80 port" in withContainers { container =>
    assert(Source.fromInputStream(
      new URL(s"http://${container.containerIpAddress}::${container.mappedPort(80)}/").openConnection().getInputStream
    ).mkString.contains("If you see this page, the nginx web server is successfully installed"))
  }

}

[dimafeng]

Hey @LMnet I'm a bit rusty on the original idea of Def. Do you remember why it wasn't added initially?

[LMnet]

Hi @alejandrohdezma

I tried to compile your example but it's not compiling. Could you fix it? After that, we will be able to discuss your idea in more detail.

[alejandrohdezma]

Hey @LMnet, thanks for the swift response? What exactly is not compiling? I just run +Test/compile and everything is working fine.

[LMnet]

I'm talking about your example in the merge request description:

class GenericContainerSpec extends AnyFlatSpec with TestContainersForAll {
  
  override val containerDef = GenericContainer.Def("nginx:latest",
    exposedPorts = Seq(80),
    waitStrategy = Wait.forHttp("/")
  )

  "GenericContainer" should "start nginx and expose 80 port" in {
    assert(Source.fromInputStream(
      new URL(s"http://${container.containerIpAddress}:${container.mappedPort(80)}/").openConnection().getInputStream
    ).mkString.contains("If you see this page, the nginx web server is successfully installed"))
  }
}

[alejandrohdezma]

Thanks @LMnet, I have updated the example and the changes. The example was using the wrong trait and missing the withContainers. I've also updated the changes since previous changes didn't allow the suite to use the container as a GenericContainer and thus unable to use methods like mappedPort, should be fixed now.

[LMnet]

Thanks, @alejandrohdezma Now it looks fine.

But I'm a bit concerned about the impact of this apply method. apply usually means something like a default constructor. But in this case, it's not true. Your use case feels like you want to use an anonymous generic container. So maybe it is worth renaming your new apply to anonymous? @dimafeng wdyt?

[alejandrohdezma]

@LMnet done!

[dimafeng]

@alejandrohdezma would you mind updating your example snippet. It seems I'm not following the idea with anonymous but an example should help

[alejandrohdezma]

Done!

[dimafeng]

Hey @LMnet quick question - why GenericContainer.Def.anonymous is better than just GenericContainer.Def? I think I'm missing something.

[LMnet]

All my concerns I described above. The default use case for GenericContainer.Def is described here.

Overall, GenericContainer.Def is a building block for a container definition for some container. But @alejandrohdezma's use case is about creating an anonymous container definition (means without corresponding class for a container).

If we will use apply instead of anonymous, I think users may misuse this API. They could think that this apply is just an another form of a default constructor, but it's not actually true.

[dimafeng]

Got it! It makes sense.

@LMnet @alejandrohdezma would it make sense to create a new class instead e.g.AnonymousGenericContainer / AnonymousContainer? In that case, it's going to be a bit more consistent with other Def's (override val containerDef = AnonymousGenericContainer.Def(...)) What do you think?

[alejandrohdezma]

Got it! It makes sense.

@LMnet @alejandrohdezma would it make sense to create a new class instead e.g.AnonymousGenericContainer / AnonymousContainer? In that case, it's going to be a bit more consistent with other Def's (override val containerDef = AnonymousGenericContainer.Def(...)) What do you think?

Of course! I'll send a new commit tomorrow morning :(

[dimafeng]

Thanks!

@LMnet does it sound good to you?

[LMnet]

@dimafeng I'm not sure. Discoverability of AnonymousGenericContainer.Def will be definitely worse, than GenericContainer.Def.anonymous.

[alejandrohdezma]

If we go with that I would remove the anonymous too, and leave it just as apply, since discoverability would be better as it would be as any other container definition, where the apply method constructs the definition. In this case we will be just creating a "generic" container definition.

[dimafeng]

Seems we're comparing several non-ideal solutions:

1. GenericContainer.Def.anonymous goes against the convention that all other Defs use
2. AnonymousGenericContainer.Def breaks the 1:1 mapping with old api
3. GenericContainer.Def uses apply non-canonically

4th option:
Should we consider renaming abstract class Def[C <: GenericContainer] to something else and then reusing it for a definition of GenericContainer.Def?

[alejandrohdezma]

Sorry @dimafeng I didn't see your reply :(

What I like about the apply idea is that it doesn't involve a breaking change (like renaming Def) and it is consistent with all the other containers, where you have PostgresContainer.Def#apply, but in this case for creating a "generic" one.

[dimafeng]

After thinking a bit, I'm leaning toward your options with Def.apply with potential breaking-change migration to the option 4 in the future. For me, consistency is pretty important for discoverability.

@LMnet any final thoughts?

[LMnet]

I prefer option 1. But option 3 is acceptable.

[dimafeng]

@alejandrohdezma can we proceed with the option 3? Would you mind also changing README and adding a simple test?

[alejandrohdezma]

Of course, I'll do it as soon as possible :)

[alejandrohdezma]

@alejandrohdezma can we proceed with the option 3? Would you mind also changing README and adding a simple test?

Done! :)

[dimafeng]

@alejandrohdezma thank you! Seems some tests failed

[dimafeng]

Can we update this section instead - https://github.com/testcontainers/testcontainers-scala#genericcontainer-usage

I'd like to keep it consistent until we finally migrate to one way of doing it.

[alejandrohdezma]

Done here: 4e8e553

[dimafeng]

@alejandrohdezma thanks! The test is failing with scala 3

 [info] compiling 13 Scala sources and 1 Java source to /home/runner/work/testcontainers-scala/testcontainers-scala/test-framework/scalatest/target/scala-3.0.2/test-classes ...
[error] -- [E008] Not Found Error: /home/runner/work/testcontainers-scala/testcontainers-scala/test-framework/scalatest/src/test/scala/com/dimafeng/testcontainers/GenericContainerSpec.scala:17:35 
[error] 17 |      new URL(s"http://${container.containerIpAddress}:${container.mappedPort(80)}/").openConnection().getInputStream
[error]    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[error]    |value containerIpAddress is not a member of GenericContainerSpec.this.Containers
[error] -- [E008] Not Found Error: /home/runner/work/testcontainers-scala/testcontainers-scala/test-framework/scalatest/src/test/scala/com/dimafeng/testcontainers/GenericContainerSpec.scala:17:67 
[error] 17 |      new URL(s"http://${container.containerIpAddress}:${container.mappedPort(80)}/").openConnection().getInputStream
[error]    |                                                         ^^^^^^^^^^^^^^^^^^^^
[error]    |value mappedPort is not a member of GenericContainerSpec.this.Containers
[error] two errors found
[error] two errors found

[alejandrohdezma]

@dimafeng should be fixed now