-
Notifications
You must be signed in to change notification settings - Fork 749
Migrate images from dockerpy to aiodocker #6252
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
|
Looking at the failed build log, I'm thinking that the idea of doing this migration in stages may not work right now. Both libraries want the same unix socket and I don't think it can work like that. We may actually have to do this as one giant PR unless we can find a way to get both running simultaneously during the migration |
8d4957c to
94d1f52
Compare
|
Ok I resolved the conflict with the unix sockets. In testing locally it seems to work fine as long as you aren't using literally the same file. So by binding one to |
Try using the same socket again.
To me this doesn't really make sense, since the two sockets are symlinked, so these end up to be the same files. Also, I would expect that the two libraries open a separate file handle, which should not cause any interference. I tried to reproduce that by using Afaik, these were the two problematic "Run the Supervisor" CI runs:
I've reverted the change again, but this time the run went though fine I did run the CI run a couple of times, it worked every time. So, its not reproducible anymore!? 🤔 😕 Maybe something was wrong with GitHub back then? |
agners
left a comment
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 makes Docker interactions of Supervisor using asyncio patterns 🤩! Just a minor nit, other than that, PR looks great! I also like the change to fire_event to return tasks and actually await them 👍.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
First in a series of PRs to migrate us from Dockerpy to Aiodocker. See #6247 for details and reasoning. This is far too much work for 1 PR so it'll be broken down into chunks.
This PR covers the following:
ImageCollectionobject to aiodockeraiodockerto our requirementsDockerAPI.dockerproperty will now return the aiodocker client object. TheDockerAPI.dockerpyproperty will be used to access the dockerpy client until it is no longer neededType of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed: