Skip to content

Conversation

@sanderegg
Copy link
Member

What do these changes do?

#8286 introduced an issue where a call to max could be made with an empty array:
image

This PR fixes it.

Related issue/s

How to test

Dev-ops

@sanderegg sanderegg added this to the Cheops milestone Sep 4, 2025
@sanderegg sanderegg self-assigned this Sep 4, 2025
@sanderegg sanderegg requested a review from pcrespov as a code owner September 4, 2025 11:23
@sanderegg sanderegg added the a:director-v2 issue related with the director-v2 service label Sep 4, 2025
@sanderegg sanderegg requested a review from GitHK as a code owner September 4, 2025 11:23
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 4, 2025

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug where the max function could be called with an empty array, which would result in a ValueError. The issue was introduced in PR #8286 where the code structure allowed for a scenario where tasks_waiting_for_cluster could be empty when passed to the max function.

  • Adds early return checks to prevent calling max on an empty list
  • Restructures the conditional logic to handle edge cases more robustly

@codecov
Copy link

codecov bot commented Sep 4, 2025

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 68.82%. Comparing base (d235084) to head (09927da).
⚠️ Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (d235084) and HEAD (09927da). Click for more details.

HEAD has 30 uploads less than BASE
Flag BASE (d235084) HEAD (09927da)
unittests 31 1
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #8308       +/-   ##
===========================================
- Coverage   85.62%   68.82%   -16.80%     
===========================================
  Files        1940      755     -1185     
  Lines       75322    34792    -40530     
  Branches     1311      175     -1136     
===========================================
- Hits        64491    23945    -40546     
- Misses      10436    10790      +354     
+ Partials      395       57      -338     
Flag Coverage Δ
integrationtests 64.16% <15.38%> (+0.08%) ⬆️
unittests 84.59% <92.30%> (+0.46%) ⬆️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 76.72% <ø> (-8.31%) ⬇️
agent ∅ <ø> (∅)
api_server ∅ <ø> (∅)
autoscaling ∅ <ø> (∅)
catalog ∅ <ø> (∅)
clusters_keeper ∅ <ø> (∅)
dask_sidecar ∅ <ø> (∅)
datcore_adapter ∅ <ø> (∅)
director ∅ <ø> (∅)
director_v2 91.06% <92.30%> (+0.05%) ⬆️
dynamic_scheduler ∅ <ø> (∅)
dynamic_sidecar 81.87% <ø> (-8.59%) ⬇️
efs_guardian ∅ <ø> (∅)
invitations ∅ <ø> (∅)
payments ∅ <ø> (∅)
resource_usage_tracker ∅ <ø> (∅)
storage ∅ <ø> (∅)
webclient ∅ <ø> (∅)
webserver 58.99% <ø> (-21.52%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d235084...09927da. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sanderegg sanderegg merged commit d7ee297 into ITISFoundation:master Sep 4, 2025
89 of 92 checks passed
@sanderegg sanderegg deleted the bugfix/max-arg-empty-sequence branch September 4, 2025 11:35
@mergify
Copy link
Contributor

mergify bot commented Sep 4, 2025

🧪 CI Insights

Here's what we observed from your CI run for 09927da.

🟢 All jobs passed!

But CI Insights is watching 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:director-v2 issue related with the director-v2 service

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants