-
Notifications
You must be signed in to change notification settings - Fork 44
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
editoast, osrdyne: fix the multi infra status response #9041
Conversation
Khoyo
commented
Sep 27, 2024
•
edited
Loading
edited
- Build up list querys correctly on editoast side
- Aggregate the worker status results correctly on editoast side
- Use a by worker_id map (instead of a by worker_key) in osrdyne response
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #9041 +/- ##
============================================
- Coverage 37.19% 37.18% -0.01%
Complexity 2242 2242
============================================
Files 1263 1263
Lines 116087 116099 +12
Branches 3278 3278
============================================
- Hits 43174 43172 -2
- Misses 70959 70973 +14
Partials 1954 1954
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
fd94026
to
d9cda62
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.
Can you put a description in the PR, I'm not entirely sure I understand what this is supposed to fix (is there any existing bug? why did the API changed? etc.)
Yes, the multi infra endpoint simply did not work (which is the reason for the request change on editoast side) :/ And we didn't support the case where there is multiple workers per key, which is coming soon with the work on using Keda, so I changed up the api a bit (we now return a map indexed by worker id, not worker key - except in the noop case where we have to make stuff up anyway) |
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 the explanations. Looks good.
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.
LGTM 👍🏽
Signed-off-by: Younes Khoudli <younes.khoudli@epita.fr>
30d7e57
to
61e2904
Compare