Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Canned container, let's kick the discussion #112

Closed
gianarb opened this issue Nov 12, 2019 · 21 comments
Closed

Canned container, let's kick the discussion #112

gianarb opened this issue Nov 12, 2019 · 21 comments
Labels
feature New functionality or new behaviors on the existing one

Comments

@gianarb
Copy link
Collaborator

gianarb commented Nov 12, 2019

Canned containers are a good way to have pre-built container that can be re-used and can support a deep implementation with the application they are running.

When I started to do them I thought it was a good idea but now I would like to avoid testcontainers to become full of third-party dependency to download. That's why at the moment I am not merging them.

Why I like canned containers:

  1. It is a good way to share the effort we do in order to build a great experience with the library
  2. It helps new contributors to get onboard

What I do not like:

  1. To many dependencies to download with the library

Do you have any suggestions?! I don't know how go mod works with submodules and if they are supported at all. One idea can be to have a foldering like:

canned/mysql
canned/etcd
canned/kubernetes
canned/postgres

And we should figure out a way to have the dependency only for that directory and not for the root o the project.

Affected issue/pr until now:

#108
#107
#105
#102
#98
#59

@gianarb gianarb added the feature New functionality or new behaviors on the existing one label Nov 12, 2019
@borgoat
Copy link

borgoat commented Nov 13, 2019

I guess the right way to do this would be to create a Git repo (and consequently Go module) for each canned product?

End users would then have to go get such modules then (test containers-go won't have them as dependency or you'd be back at stage 1)

@gianarb
Copy link
Collaborator Author

gianarb commented Nov 13, 2019

One repository for every canned dashboard is to expensive and it does not help with visibility. I don't think we can go with that solution. What I think is the way to go is using go submodules

https://github.com/go-modules-by-example/submodules

Where we can have a canned/mysql directory for example with its own files and its own go.mod.

@nikolayk812
Copy link

I agree with @gianarb, repo per canned container sounds like an overkill.
If submodules approach work then this is probably a way to go.

@borgoat
Copy link

borgoat commented Nov 14, 2019

I had no idea this could be done! TIL
https://golang.org/cmd/go/#hdr-Module_code_layout confirms that this should work.. I guess we can give it a go?

@gianarb
Copy link
Collaborator Author

gianarb commented Nov 14, 2019 via email

@ClaytonNorthey92
Copy link
Contributor

ClaytonNorthey92 commented Nov 14, 2019

I am indifferent with where the code for canned containers lives, I think a submodule makes sense to try.

One concern I do have, it seems like these canned containers don't have a common interface? Or am I missing it? We have a name for "canned containers", but there is really nothing in code that defines what it takes to be a "canned container".

I think we should define some interface that makes something a "canned container".

The most obvious problem I see in the future is the versioning. I think any canned container should support a default version, but also allow the user to set a version they want. For a redis canned container, for example the default version might be 5.0, and this would change over time, but if I am a consumer I might want to keep it at 5.0.6-alpine for as long as I can.

so to define this...

type CannedContainer interface {
    DefaultVersion() string
    SetVersion(version string) error
    StartContainer() (DockerContainer, error)
}

the point I am trying to make is that I think (if we don't do so already) we should define a CannedContainer interface at least

what do you all think? if you agree, what other common methods should we include?

@mdelapenya
Copy link
Collaborator

+1 on having a clean interface for canned containers, it would help in the development of new cans.

And related to packaging, I'd go with a package for cans, having a Go file per can. Anybody will be able to use github.com/testcontainers/testcontainers-go/canned as a dependency, exporting methods properly

@gianarb
Copy link
Collaborator Author

gianarb commented Nov 19, 2019

@mdelapenya from your message I do not understand if you would like to have also a go.mod for every single canned container (I probably would like to to get there as well). In my opinion, the canned container should at least implement the Container interface we have already, other methods such as GetClient (to get a pre-populated client to interact with the container) may or may not be provided if needed so it should be enforced.

What do you think a canned container should expose that is not exposed by the Container interface?

@ClaytonNorthey92
Copy link
Contributor

ClaytonNorthey92 commented Nov 19, 2019

I think that the CannedContainer interface should at least expose a way to get the Version, and maybe SetVersion?

so, if someone wanted to create an Envoy container, their workflow might be something like...

envoyContainer := canned.NewEnvoyCannedContainer()
desiredVersion := "v1.12.0"
if envoyContainer.Version() != desiredVersion {
    envoyContainer.SetVersion(desiredVersion)
}
envoyContainer.Start(context.Background())
...

I would like to ensure that the user has the ability to change the CannedContainer's version, and (this is a bit harder I think) ensure that CannedContainers don't develop a business-use-case-specific or version-specific API per image

@mdelapenya
Copy link
Collaborator

@mdelapenya from your message I do not understand if you would like to have also a go.mod for every single canned container (I probably would like to to get there as well). In my opinion, the canned container should at least implement the Container interface we have already, other methods such as GetClient (to get a pre-populated client to interact with the container) may or may not be provided if needed so it should be enforced.

What do you think a canned container should expose that is not exposed by the Container interface?

Well, in an ideal situation, that would be great: each canned container, a version, which would enforce strong contracts between client code and the canned containers. Another thing is how much manual work we need to do to maintain it. On the other hand, when doing development mode of the testcontainers-go library we can replace the dependencies to use the project itself, which would simplify that.

About the common interface, if the Container already supports a clean one, I'm good with it.

@ClaytonNorthey92
Copy link
Contributor

ClaytonNorthey92 commented Nov 19, 2019

oh an sorry, @gianarb yes I agree the CannedContainer interface should implement the Container interface, so I think it would look something like:

type CannedContainer interface {
    Container
    Version() string
    SetVersion(string)
}

just a rough draft ☝️

@nikolayk812
Copy link

To many dependencies to download with the library

@gianarb, good point. I think we should try to minimize dependencies a new canned container introduces. In order to start a Postgres container probably, we don't need to add dependency at Postgres client at all.

Many customizations could be done environment variables already, waiting via proper host port waiting strategy in combination with log waiting strategy.

Even we can SQL initialization scripts if we create another container on-the-fly and copy scripts there to docker-entrypoint-initdb.d directory.

@nikolayk812
Copy link

nikolayk812 commented Nov 20, 2019

Btw, testcontainers-go already depends on Redis and MySQL clients :(

@rnorth
Copy link
Member

rnorth commented Nov 20, 2019

Aha, I think this is where I should chime in with our experiences of this on the Java side! Please take from this what you want:

  • We tried repo per module ('canned container') - it was a nightmare:

    • sometimes we'd make a change in core, then only later found out it broke one of the modules. A unified test suite is slow and heavy, but gives us way more confidence and has stopped us making so many silly mistakes.
    • rippling out changes from core to all the libraries and releasing all of them was a huge amount of effort. It didn't ever happen consistently, despite our best intentions and attempts to automate it.
    • we had different version numbers for the core and module libraries. This made sense when the releases were independent, but ultimately was just confusing for users, especially those with a dependency on more than one. It may be more pure to only increment a library's version number when it strictly has changed, but in practice I think people care more for it to be simple and predictable than perfect. We received many user complaints before unifying our versioning; not a single complaint after.
  • Regarding having to ship transitive dependencies (drivers, clients, etc), we learned over time:

    • expect your consumers to provide their own. For example, if someone is building a service that talks to Redis, they're surely going to have their own Redis client dependency, and that's the only one that matters.
    • Therefore, during the build process we might test with the latest Redis/MySQL/... library, but we do not let it become part of the transitive dependency chain we expose.
    • If you compile against/pull in a different client version and expose its types across the API boundary, pain is inevitable. Parameters or return values should really only be primitive types, or types from the 'standard library' of the language. We have a couple of modules that were created before we really learned this, and some have been really quite problematic.

These points, plus a lot of other things we learned, are in our contributing new modules docs. Please take a look - I hope this will be useful both in terms of how you implement this and how you maintain it!

@bsideup, @kiview, anything I've missed?

@nikolayk812
Copy link

@rnorth thanks for your input about!

What is your opinion about the default version of canned containers/modules?
Java version comes with a Postgres module which has version 9.6.12 under the hood. These days it is not even the latest minor version on this branch. But now if you change it to version 12.x it would be good for new users, but for me, it would be unexpected.

@rnorth
Copy link
Member

rnorth commented Nov 22, 2019

Yeah, that's a really good question. Changing the version would be unexpected, as you say, so we can't do that. We're probably locked in to these 'starter' versions at least until we release a new major version of the libraries.

It's obviously a good idea for users to specify the exact image they want; in the next major version I wonder if we might remove the default versions altogether.

I think I'd suggest having a mandatory parameter for the docker image name+tag - it is a little inconvenient for new users, but probably better in the long term. New users will often be reading or copying from documentation, so this does perhaps reduce the inconvenience a bit.

BTW, allowing the docker image to be chosen by the user is a double-edged sword. For example, we had quite an adventure finding connection/driver settings for MySQL that work with MySQL versions from 5 through 8+ 😂 .

@ClaytonNorthey92
Copy link
Contributor

ClaytonNorthey92 commented Nov 22, 2019

Yeah, that's a really good question. Changing the version would be unexpected, as you say, so we can't do that. We're probably locked in to these 'starter' versions at least until we release a new major version of the libraries.

It's obviously a good idea for users to specify the exact image they want; in the next major version I wonder if we might remove the default versions altogether.

I think I'd suggest having a mandatory parameter for the docker image name+tag - it is a little inconvenient for new users, but probably better in the long term. New users will often be reading or copying from documentation, so this does perhaps reduce the inconvenience a bit.

BTW, allowing the docker image to be chosen by the user is a double-edged sword. For example, we had quite an adventure finding connection/driver settings for MySQL that work with MySQL versions from 5 through 8+ 😂 .

@rnorth would the interface I mentioned above be a step in the correct direction to address the version problem?

@nikolayk812
Copy link

@ClaytonNorthey92 I have checked your suggested interface. By the way, we have two types of entities in this lib: request and container. So we should set a version to request and from the container we can only get the effective version.

If you have time, I would appreciate some feedback on #117. I have reworked a bit PRs by @giorgioazzinnaro

@gianarb
Copy link
Collaborator Author

gianarb commented Dec 3, 2019

I had this missing feeling already when I started this conversation but I think from what @rnorth says I see it getting stronger. Should we really go into the rabbit hole of modules, canned container?

They are good for onboard but a pain to code and maintain, as a plus it is very hard to find an interface that is flexible enough but not complex (otherwise you will just use to not canned one).

So, should we treat this problem as a documentation issue?

@jaredpetersen
Copy link
Contributor

It looks like the debate between submodules and documentation updates hasn't been totally settled yet.

One thing I've noticed with the equivalent on Java Testcontainers is that the custom containers provided by the various libraries tend to be coupled to a particular image and version while pretending that you can still just pass in whatever image you want. Some set of environment variables may be specified for you already to make configuration easier but those configurations don't necessarily work for all versions.

Additionally, the library containers seems to be just enough to work but when you want to do something that's a little custom (e.g. configure Redis to run in cluster mode instead) it falls apart.

I think the documentation route is a better approach here. We can provide some samples on how to set up the containers but ultimately leave the implementation up to the user. If people are clamoring for it, we can always revisit providing official implementations later. But I think that the documentation route is the simplest and quickest path to helping users with this now.

I'd be happy to get the ball rolling with a PR. I have a Redis example that I can share and can probably come up with a Kafka + Schema Registry one as well, which I know has been requested before (#246).

@mdelapenya
Copy link
Collaborator

I think this thread belongs to the Discussions section much better than here. I'll move it there

@testcontainers testcontainers locked and limited conversation to collaborators Aug 25, 2022
@mdelapenya mdelapenya converted this issue into discussion #509 Aug 25, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

No branches or pull requests

7 participants