Add integration test CI task and fix integration test #489
Conversation
|
To do:
|
.circleci/config.yml
Outdated
| - run: docker swarm init # this should be remove when no test are actually using docker | ||
| - run: docker build -t sleep docker-images/sleep/ # this should be remove when no test are actually using docker | ||
| - run: docker build -t http-server docker-images/http-server/ # this should be remove when no test are actually using docker | ||
| - run: env MESG_CORE_IMAGE=mesg/core:NONEXISTINGIMAGE go test -timeout 180s -p 1 -coverprofile=coverage.txt ./... |
There was a problem hiding this comment.
What about syntax:
run:
name:
command
envrioment:
I think is more clear than putting everything in one line
There was a problem hiding this comment.
We don't have the env variable anymore and CircleCi doesn't provide to anyway to pass arguments through a nice yaml list.
I'm ok to read the command and not a short name.
What do you think @krhubert?
This parameter tell what event from Docker to wait to confirm the deletion of the network. Can be "remove" or "destroy" depending in context.
# Conflicts: # service/start_test.go # service/stop.go # service/stop_integration_test.go
NicolasMahe
changed the title
|
@mesg-foundation/core ready to review |
| // event parameter can be "destroy" or "remove". If the network was used by a service, the event to use is "destroy". If the network has not been used, the event is "remove". | ||
| // Remove removes the reference from Docker to the network. | ||
| // Destroy removes the network from Docker active network. | ||
| func (c *Container) DeleteNetwork(namespace []string, event EventType) error { |
There was a problem hiding this comment.
it's a bit odd to me having event as an argument to this func because it does not change the behavior of network removing. we can listen for both events and return after the first received event. this way use of this func will be agnostic and caller will not need to figure out which event type it should pass.
There was a problem hiding this comment.
The problem with this one is that we cannot wait for both. In some cases we will have both (when we create the service and attach the network to this service) but in some other cases when we just create and destroy the network without any service we only have the "remove" event.
I agree this is kind of weird to have this event maybe we can rename it to deletionEvent or something like that.
I don't see any ways to make this function agnostic because it's really related to docker and how we use it but what thing that can be done in the future is to remove this CreateNetwork and DeleteNetwork from the public API and manage the network internally in the container package which totally makes sense.
There was a problem hiding this comment.
Totally agree with the last part.
This is not a perfect solution but the perfect solution requires to reorganize the container package to match our logic (mesg's service have multiple docker's service, one network) rather than staying close to the docker logic (service, container, network). By doing this, we could extract a lot of docker related logic from the service package to the container package (like start and stop mesg's service with dependencies and network).
There was a problem hiding this comment.
i don't quite get the limitation of waiting for both destory and remove here. does docker produces two events for both destory and remove when we have a service attached to network?
There was a problem hiding this comment.
Yes it does. But as we run integration test that only create a network and them delete it, we need a way to change the behaviour of DeleteNetwork otherwise we have timeout and the integration test on CreateNetwork doesn't work (as well as others).
There was a problem hiding this comment.
That's why, after transferring more logic to the container package (where we ALWAYS create a network attached to service), we will be able to remove the event parameter (and directly this public function).
There was a problem hiding this comment.
I understand now. maybe we should inspect the network to see if there is any services attached to it before removing, this way we can guess the event type fore network removing without doing changes on container package. but there can be a race condition with this approach, while we're inspecting the network some services can be removed or started.
There was a problem hiding this comment.
i think, the race condition can be prevented by also listening for service start/stop events in this func.
NicolasMahe
dismissed stale reviews from krhubert and antho1404
via
14eb4d6
Fix change of behavior of StopService introduced in commit #0366e06
This PR was meant to be only to add integration test on CircleCI but as integration test failed I had to fix them.
Closes #445