Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add integration test CI task and fix integration test #489
Add integration test CI task and fix integration test #489
Changes from 26 commits
d638710
35520e3
1c9947e
f691d4d
021cafc
5fa0f70
db29f3f
05eaf50
13242d6
2519f0d
f0b2d25
3b5e850
7e7260c
08506b5
04d6596
373b146
08abd98
4cb4010
45ff0d3
36e908a
c7b5004
c8da0e2
27f83b5
6ff1c97
c26e98d
e627dba
14eb4d6
9ae3322
a1cff3e
461132a
8b80517
e452858
9816b5e
2811133
cbc69c3
829699e
ed46533
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 todeletionEvent
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
andDeleteNetwork
from the public API and manage the network internally in the container package which totally makes sense.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.
Totally agree with the last part.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 onCreateNetwork
doesn't work (as well as others).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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think, the race condition can be prevented by also listening for service start/stop events in this func.