-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add events method to docker clients #9932
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
Conversation
LocalStack Community integration with Pro 2 files 2 suites 1h 18m 21s ⏱️ Results for commit 2af4b6e. ♻️ This comment has been updated with latest results. |
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 can help us significantly, but we need to make the streams cancellable, to avoid resource leaks.
for msg in CancellableProcessStream(process): | ||
try: | ||
yield json.loads(msg) | ||
except json.JSONDecodeError as e: | ||
LOG.warning("Error decoding docker event %s: %s", msg, e) | ||
|
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.
Not sure about this. The stream is not cancellable with this construct. I think for these method we should always use a CancellableStream, so we can cancel it if necessary.
c44f20e
to
1f3ad85
Compare
1f3ad85
to
2af4b6e
Compare
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.
Thanks for making it cancellable!
Motivation
For an upcoming change to the way containers are handled, we will want to be able to capture the docker events in order to properly handle notifications of container termination or shutdown.
Changes
This PR adds an
events
method, which returns a generator of the events from the docker daemon.