-
-
Notifications
You must be signed in to change notification settings - Fork 478
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
feat: Add DependsOn support #2035
base: main
Are you sure you want to change the base?
feat: Add DependsOn support #2035
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hey @Mathew-Estafanous thanks for submitting this PR. Wdyt if we first create a GH discussion about the design of this feature? As an example, I had in mind having a way to detect cycles in the dependsOn: i.e. srv1 depends on srv2, but what happens if srv2 depends on srv1 (users doing crazy things?) how would we handle that error? In that sense, feel free to create the discussion so we can elaborate a great design. Please take a look at #559 as an example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks, been following from otel side, this will be super useful for us.
@ preventing circular dependencies, sounds reasonable but not sure it should be a blocker, can always followup with such in a different PR circular dependencies are inherently un-resolvable so imho it wouldn't change the contract API @ design I would be okay with adding dependson as another (also, probably should be a set rather than array but idk if it's a hashable type and golang set semantics are fairly arduous so for sake of simplicity I'm okay with not enforcing such) |
Thanks for reviewing! I appreciate it.
Come to think of it, given the current API design, how could someone create a circular dependency? You'd already need to have created both containers. Am I missing something? |
Given That said, I suppose that is a design consideration (whether DependsOn should take in actual instantiated containers or if they should be dependent upon other container requests). I see in the It's still an improvement but imho having container requests depending on a container name/some other reference instead of the go object would be preferable. |
I think you're aligned with "upstream" (assuming the java impl is the reference spec for testcontainers) Looks like all their dependsOn are of type that said, I don't think java has the notion of containerrequest to begin with, so I think that puts the ball of design back in our court. |
Agreed, it would be quite the feat to shoe-horn such a blatant anti-pattern in the current design. But you do bring up a good point on maybe generalizing the api to take in something less concrete than a Container. I think referencing just a name/container ID might not be enough considering the need to respect each container's |
Perhaps we can make the type of DependsOn a custom type/interface (pseudo-type union) we could then implement it as-is and extend in the future? FWIW docker-compose just uses a reference to the container in |
Not sure what you mean by make |
Sorry. So, currently, That said, theoretically anything that implements |
a6ca699
to
3d0886e
Compare
3d0886e
to
f81c84c
Compare
Hi folks, sorry for the radio silence during this month but Xmas holidays and company trips made it difficult to me to jump in without all the context. First thank you all for the great community work you've been doing here with your proposals 🙇 I've enjoyed a lot seeing you collaborating here. And now to the topic. The library defines two different concepts for a container:
So, coming back to the
The 1) option would be simpler, but less developer friendly, as I'd be forcing developers to not start the supporting containers; then the library, using the lifecycle hooks, would start them before containerA. The 2) option seems more developer friendly, as I'd declare all the container requests, would set the DependsOn dependencies, and would start the containerA. Then the library, using the lifecycle hooks, would start all the dependencies. I see this PR more aligned with option 1, which fairly works, although I see it less easy to use. At least at the moment. For options 2, we would need to depend on another container request, but still relying on the users not calling the GenericContainer(...) method on the dependent containers before calling So, what would you prefer? |
I agree that the implemented solution isn't as easy to use and I think it would be be better to work on a solution similar to your proposed 2nd solution. However, I'm concerned that with the use of We could introduce a type Dependency struct {
ContainerRequest ContainerRequest
OnStarted func(Container) error
} It's not the most elegant solution, but it offers a workaround in gaining access to the Alternatively, (if not already possible) we could consider introducing a 'find' container mechanism, possibly similar to the internal What are your thoughts? Is having access to dependant containers necessary? |
Hey @mdelapenya I just want to bump this conversation. Have you had time to review my comments? I understand if this on hold due to scheduling. Thanks! |
Hi. As a potential user of this feature, first at all thanks @Mathew-Estafanous for moving this forward. I tend to agree with @mdelapenya that using containers is not natural in this context, mostly because the feature will automatically start dependencies if they are not already started.
I think in general it is needed. For instance, an application (container What I don't see is how this could work, as we need this address before starting the container The way Docker compose solves this is by injecting the dependencies as environment variables. In my example above, the
I think this could work better, but still, I don't see how this would work in the case I described above, |
Hey @pablochacin, thanks for your input! And sorry for not getting back earlier.
You make a good point. We can enable a similar behaviour by adding all dependant containers and the parent to the same network. This should allow the parent (or dependant) container to reference each other's IP address through the use of docker's built-in DNS service. That should allow for calls like What do you think? Would that suffice in this case? |
I think putting the containers in the same network and using a fixed name for the containers could work, but with that many requirements, the solution looks fragile and somehow forced. I think the callback you proposed could work as a more robust mechanism. if it is called after the dependency is running but before the container with the dependencies is created, we can inject any parameter we need in the container request, host names, environment variables, etc. I lean towards this initial approach and enhance developer experience by providing some options that implement frequent use cases like:
|
I don't believe we'd necessitate the use of fixed names. We could have a callback similar to For example. ...WithDNSDependency(myRedisService, "REDIS_HOSTNAME") On creation of the Internally, the containers would be created in a way similar to this: > docker network create testcontainers
# start dependent redis container
> docker run -itd --network=testcontainers redis:latest
# image named 'youthful_elgamal' is created
> docker run -itd --network=testcontainers -e REDIS_HOSTNAME=youthful_elgamal caching_service:latest
# creates `caching_service` container which can rely on DNS resolution based on the environment variable passed to it. What do you think? Does this work or am I misunderstanding your concerns? |
@Mathew-Estafanous I explained poorly. I meant that if we don't introduce a mechanism like the one I suggested, we would need fixed names. With the options I suggested, that is not necessary, as you explained. So, in summary, I think implementing the dependency with a callback is a good first step and the the options for automatically setting DNS or ENV with the container's IP can be built on top of it. |
f81c84c
to
da11872
Compare
dependency_test.go
Outdated
NewContainerDependency(nginxReq, "FIRST_DEPENDENCY"), | ||
NewContainerDependency(nginxReq, "SECOND_DEPENDENCY"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the interface that I came up with for dependencies. I still have to add more tests and work on edge cases, but thought I'd push this up to get some eyes on it.
net, err := GenericNetwork(ctx, GenericNetworkRequest{ | ||
NetworkRequest: NetworkRequest{ | ||
Driver: Bridge, | ||
Labels: GenericLabels(), | ||
Name: fmt.Sprintf("testcontainer-dependency-%v", uuid.NewString()), | ||
Internal: false, | ||
}, | ||
}) | ||
depNetwork = net.(*DockerNetwork) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping to use network.New
for this, but that causes a cyclical dependency - which is why I'm using these depreciated functions.
I'd appreciate any suggestions on how to use network.New
instead of doing this.
// CallbackFunc is called after the dependency container is started. | ||
CallbackFunc func(Container) | ||
// KeepAlive determines whether the dependency should be kept alive after the parent container is terminated. | ||
KeepAlive bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a KeepAlive
flag since I thought it'd be helpful to define whether or not the dependency should terminate alongside its parent.
I set the default to true
in NewContainerDependency
but I'd be interested to hear other's opinions of the flag itself and if true
is an appropriate default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think should be consider later. I can see the potential if there where multiple sets, but that might be better served by references instead of just a keepalive flag, so would suggest we remove this for now.
0a91d23
to
a2f0c45
Compare
// ContainerDependency represents a reliance that a container has on another container. | ||
type ContainerDependency struct { | ||
Request ContainerRequest | ||
EnvKey string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The environment variable can be injected by the callback function and not all containers can take advantage of it, so I'm not convinced this should be implemented in the dependency struct as a default behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, i'm not sure what you mean by the env variable can be injected by the callback func. The container would be already running by the time the callback is called and I'm not aware if injecting env vars into a live container would be possible at that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean that we should make Env injection optional through the use of something like WithEnvName(envKey string)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The container would be already running by the time the callback is called
The dependency will be running, but not the container that depends on it. See please an additional note that I made about the callback function signature.
Do you mean that we should make Env injection optional through the use of something like
WithEnvName(envKey string) ?
Yes, that was my original proposal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Is there any reason why the env would be a requirement?
@Mathew-Estafanous Overall I like the approach. My only observation is that I think injecting the environment variable by default is not necessary. The callback function can do this. I would remove that from the interface. |
Request ContainerRequest | ||
EnvKey string | ||
// CallbackFunc is called after the dependency container is started. | ||
CallbackFunc func(Container) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the callback should receive also the container request of the container that has the dependency. In this way the callback can modify the request (e.g. add env variables, setup the hosts file, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds sensible to me
suggestion: return an error too given request methods do this now.
@pablochacin I discussed this issue at length with Manuel and wrote a quick explanation of what was decided on in the DependsOn discussion post. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clearly well on the way and would be a great addition, so with the goal of supporting to get this over the line, I've done a pass with some feedback.
I've been doing a bunch of PRs, but still I'm new around here and not everyone will have encountered them, so calling out I use conventional comments to try and ensure that the intent is consistent and well understood.
On of the key things we should try to do is keep the functionality minimal, seems there's been a bit a feature creep for example, keepalive which could delay getting it over the line so might want to keep that for a follow one after more discussion.
Hope this helps.
|
||
// ContainerDependency represents a reliance that a container has on another container. | ||
type ContainerDependency struct { | ||
Request ContainerRequest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: These should be private to ensure consumers don't change after creation.
// ContainerDependency represents a reliance that a container has on another container. | ||
type ContainerDependency struct { | ||
Request ContainerRequest | ||
EnvKey string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Is there any reason why the env would be a requirement?
Request ContainerRequest | ||
EnvKey string | ||
// CallbackFunc is called after the dependency container is started. | ||
CallbackFunc func(Container) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds sensible to me
suggestion: return an error too given request methods do this now.
// CallbackFunc is called after the dependency container is started. | ||
CallbackFunc func(Container) | ||
// KeepAlive determines whether the dependency should be kept alive after the parent container is terminated. | ||
KeepAlive bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think should be consider later. I can see the potential if there where multiple sets, but that might be better served by references instead of just a keepalive flag, so would suggest we remove this for now.
} | ||
} | ||
|
||
func (c ContainerDependency) WithKeepAlive(keepAlive bool) ContainerDependency { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Lets get some comments for all exported methods.
} | ||
}() | ||
|
||
net, err := GenericNetwork(ctx, GenericNetworkRequest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Do we need to enforce a separate network?
err := depNetwork.provider.client.NetworkConnect(ctx, depNetwork.ID, container.GetContainerID(), nil) | ||
defer depNetwork.provider.Close() | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: check and wrap
err := depNetwork.provider.client.NetworkConnect(ctx, depNetwork.ID, container.GetContainerID(), nil) | |
defer depNetwork.provider.Close() | |
return err | |
defer depNetwork.provider.Close() | |
if err := depNetwork.provider.client.NetworkConnect(ctx, depNetwork.ID, container.GetContainerID(), nil); err != nil { | |
return fmt.Errorf("connect network: %w", err) | |
} | |
return nil |
expectedEnv []string | ||
expectedError string | ||
} | ||
testCases := []TestCase{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: There's lots of complexity here in the actual table and more in the main flow because of the conditional behaviour which actually results in more code than splitting into dedicated sub sets with t.Run(...)
We should always use succinct test names, avoiding spaces as they are replaced on the output making failures harder to track down.
Here's an example from the last item in the table.
t.Run("empty-key", func(t *testing.T) {
nginxReq := ContainerRequest{
Image: nginxAlpineImage,
ExposedPorts: []string{nginxDefaultPort},
WaitingFor: wait.ForListeningPort(nginxDefaultPort),
}
dependency := NewContainerDependency(nginxReq, "")
c, err := GenericContainer(ctx, GenericContainerRequest{
ContainerRequest: tc.containerRequest,
Started: true,
DependsOn: []ContainerDependency{dependency},
})
require.EqualError(t, err, "cannot create dependency with empty environment key")
})
If there is a block of the validation logic that's repeated extract that to a helper which continues to keep things easy to follow, reducing duplication.
{"dependency is terminated when KeepAlive is set to false", false, false}, | ||
{"dependency is still running when KeepAlive is set to true", true, true}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: succinct test names.
container, err := testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{ | ||
ContainerRequest: testcontainers.ContainerRequest{ | ||
/* Other options */ | ||
Name: "my-web-app" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: miss indented.
Name: "my-web-app" | |
Name: "my-web-app" |
Thanks for the feedback @stevenh. Admittedly I've been swamped with work and other tasks that I haven't had the chance to seriously sit down and work on this. I'll try finding time these next weeks to implement your feedback. Feel free to push your own changes if you'd like to get the ball rolling. |
func NewContainerDependency(containerRequest ContainerRequest, envKey string) ContainerDependency { | ||
return ContainerDependency{ | ||
Request: containerRequest, | ||
EnvKey: envKey, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is specific to an use case (setting an environment variable with the IP address of the dependency) and can be implemented by the call-back function, so in think shouldn't be part of the generic API.
redisDep := testcontainers.NewContainerDependency(testcontainers.ContainerRequest{ | ||
Image: "docker.io/redis:latest", | ||
/* Other options */ | ||
}, "REDIS_NAME") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think options like the environment variable should be implemented following the functional option patter:
redisDep := testcontainers.NewContainerDependency(
testcontainers.ContainerRequest{
Image: "docker.io/redis:latest",
},
/* Other options */
SetEnvVariable( "REDIS_NAME"),
)
What does this PR do?
This PR adds
DependsOn
field toContainerRequest
which can be used to supply a list of containers that must be running prior to starting the current container. On startup of the container, all containers it depends on will be started (if they are not already running).Dependencies are started within the same network such that the parent container is able to connect to its dependencies.
Dependencies are added to the parent's container request as follows:
The supplied environment variable (ie
MY_DEPENDENCY
) will be injected into the parent container and can be used to resolve the dependency's IP address.Why is it important?
Related issues
How to test this PR
A test suite for this new field accompanies the PR. See dependency_test.go