-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Set up wireguard network for task worker from manager container #21
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
- Coverage 76.52% 69.62% -6.91%
==========================================
Files 25 27 +2
Lines 656 711 +55
Branches 57 64 +7
==========================================
- Hits 502 495 -7
- Misses 140 202 +62
Partials 14 14 ☔ View full report in Codecov by Sentry. |
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.
Thank you ; that's quite the PR!
I did not actually run the code ; will do in a later review.
When refactoring, it's better to use git mv
first, commit and then do your edits (I see you did it for unedited ones). Here we have new files (that are not new) and deleted files (that have not been deleted).
It's OK for this one, project is still young and history is not that important on those files.
For the backend CLI scripts, you have coupled the implementation with the argument parsing. update_mirrors
and others now take a Namespace
as args.
I think we should have a tiny entrypoint that takes the namespace and calls a business-only function with appropriate, clearly defined/types/exposed params (if any).
manager must be hardened. At no point should anything raise an unexpected exception that would have it halt ; because it's unsupervised. This is verbose and annoying and that's one of the reason we want it as dumb as possible.
docker commands that queries the API should update values first. For instance, if you query status, attrs, etc, those are data that change overtime but which are cached in dockerpy. You want to make sure you reload()
before reading this
Thanks for your feedback on the initial changes. A lot of the changes have made the code better, I think. I have marked some of the changes as resolved and the ones I left unresolved are cases where I did things a bit differently. In summary, the things that are a bit new are:
|
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 very good ; thank you
Rationale
Set up wireguard network for task woker from manager container
Changes
manager
andtask
codemirrors-qa-backend
which takesupdate-mirrors
,create-worker
,scheduler
as subcommands. E.g,mirrors-qa-backend update-mirrors
updates the mirrors list.curl
inbackend
Dockerfile. Needed to enablehealthcheck
asworker_manager
often starts when mirrors are being updated. To be removed as things progress.dev
folder.task_worker