Skip to content

✨ Check for associated jobs when deleting a function #8342

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Sep 10, 2025
Merged

✨ Check for associated jobs when deleting a function #8342

merged 7 commits into from
Sep 10, 2025

Conversation

@giancarloromeo
Copy link
Contributor

@giancarloromeo giancarloromeo commented Sep 9, 2025 •
edited
Loading

What do these changes do?

This PR introduces functionality to check for associated jobs when deleting a function, preventing accidental deletion of functions that have active jobs. The implementation adds a force parameter to allow deletion even when jobs exist.

  • Adds validation to prevent deletion of functions with associated jobs unless forced
  • Introduces a new FunctionHasJobsCannotDeleteError exception for better error handling
  • Implements a force query parameter to allow forced deletion when needed

Related issue/s

How to test

cd services/web/server
pytest -vv --pdb tests/unit/with_dbs/04/functions/test_functions_controller_rest.py::test_delete_function_with_associated_jobs

Dev-ops

@giancarloromeo giancarloromeo added this to the Cheops milestone Sep 9, 2025
@giancarloromeo giancarloromeo self-assigned this Sep 9, 2025
@giancarloromeo giancarloromeo added the a:webserver webserver's codebase. Assigning the area is particularly useful for bugs label Sep 9, 2025
@codecov
Copy link

codecov bot commented Sep 9, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.76%. Comparing base (96d5805) to head (a5f6590).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #8342      +/-   ##
==========================================
+ Coverage   84.69%   89.76%   +5.06%     
==========================================
  Files        1945     1530     -415     
  Lines       75529    63279   -12250     
  Branches     1312      499     -813     
==========================================
- Hits        63973    56800    -7173     
+ Misses      11161     6350    -4811     
+ Partials      395      129     -266
Flag Coverage Δ
integrationtests 64.00% <25.00%> (-0.05%) ⬇️
unittests 88.17% <90.90%> (+3.59%) ⬆️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library 93.09% <100.00%> (+<0.01%) ⬆️
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration 70.19% <ø> (ø)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 85.03% <ø> (-0.06%) ⬇️
agent 93.53% <ø> (ø)
api_server 91.91% <ø> (ø)
autoscaling 95.77% <ø> (ø)
catalog 92.34% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.81% <ø> (-0.57%) ⬇️
datcore_adapter 97.94% <ø> (ø)
director 75.90% <ø> (ø)
director_v2 90.96% <ø> (+12.64%) ⬆️
dynamic_scheduler 96.27% <ø> (ø)
dynamic_sidecar 90.46% <ø> (ø)
efs_guardian 89.62% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.61% <ø> (ø)
resource_usage_tracker 92.24% <ø> (+0.10%) ⬆️
storage 86.57% <ø> (+0.08%) ⬆️
webclient ∅ <ø> (∅)
webserver 87.99% <87.50%> (+6.40%) ⬆️

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 96d5805...a5f6590. 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.

@mergify
Copy link
Contributor

mergify bot commented Sep 9, 2025 •
edited
Loading

🧪 CI Insights

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

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on master Retries 🔍 CI Insights 📄 Logs
CI unit-tests Base branch is healthy, but retries were needed. Could be early signs of flakiness 👀 Healthy 1 View View

Copilot AI reviewed Sep 10, 2025
View reviewed changes
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 introduces functionality to check for associated jobs when deleting a function, preventing accidental deletion of functions that have active jobs. The implementation adds a force parameter to allow deletion even when jobs exist.

  • Adds validation to prevent deletion of functions with associated jobs unless forced
  • Introduces a new FunctionHasJobsCannotDeleteError exception for better error handling
  • Implements a force query parameter to allow forced deletion when needed

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test_functions_controller_rest.py Adds test case for function deletion with associated jobs
conftest.py Creates test fixture for function with associated job
_functions_service.py Adds force parameter to delete_function service method
_functions_repository.py Implements job count check and force deletion logic
_functions_rest_schemas.py Defines query parameter schema for force deletion
_functions_rest.py Integrates force parameter into REST endpoint
openapi.yaml Updates API specification with force query parameter
functions_errors.py Defines new error for deletion conflicts
_functions.py Updates API spec dependencies

@giancarloromeo giancarloromeo marked this pull request as ready for review September 10, 2025 09:55
wvangeit
wvangeit approved these changes Sep 10, 2025
View reviewed changes
Copy link
Contributor

@wvangeit wvangeit left a comment

Choose a reason for hiding this comment

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

Looks good, thx.

odeimaiz
odeimaiz approved these changes Sep 10, 2025
View reviewed changes
Copy link
Member

@odeimaiz odeimaiz left a comment

Choose a reason for hiding this comment

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

thanks

sanderegg
sanderegg approved these changes Sep 10, 2025
View reviewed changes
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

thanks!

pcrespov
pcrespov approved these changes Sep 10, 2025
View reviewed changes
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

@odeimaiz odeimaiz mentioned this pull request Sep 10, 2025
Merged
@giancarloromeo
Copy link
Contributor Author

@Mergifyio queue

@mergify
Copy link
Contributor

mergify bot commented Sep 10, 2025 •
edited
Loading

queue

🟠 Waiting for conditions to match

  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed
  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = integration-tests
        • check-neutral = integration-tests
        • check-skipped = integration-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images
      • any of: [🛡 GitHub branch protection]
        • check-success = SonarCloud Code Analysis
        • check-neutral = SonarCloud Code Analysis
        • check-skipped = SonarCloud Code Analysis

@giancarloromeo giancarloromeo added the 🤖-automerge marks PR as ready to be merged for Mergify label Sep 10, 2025
@giancarloromeo giancarloromeo enabled auto-merge (squash) September 10, 2025 12:48
@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@giancarloromeo giancarloromeo changed the title ✨ Check associated jobs when deleting a function ✨ Check for associated jobs when deleting a function Sep 10, 2025
@giancarloromeo giancarloromeo merged commit 50c2568 into ITISFoundation:master Sep 10, 2025
143 of 148 checks passed
@giancarloromeo giancarloromeo deleted the is1983/check-associated-jobs-when-deleting-functions branch September 10, 2025 13:32
@pcrespov pcrespov mentioned this pull request Sep 11, 2025
Closed
16 tasks
This was referenced Sep 18, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@wvangeit wvangeit wvangeit approved these changes

@pcrespov pcrespov pcrespov approved these changes

@odeimaiz odeimaiz odeimaiz approved these changes

@sanderegg sanderegg sanderegg approved these changes

@GitHK GitHK Awaiting requested review from GitHK GitHK is a code owner

@matusdrobuliak66 matusdrobuliak66 Awaiting requested review from matusdrobuliak66 matusdrobuliak66 is a code owner

@bisgaard-itis bisgaard-itis Awaiting requested review from bisgaard-itis

Assignees

@giancarloromeo giancarloromeo

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:webserver webserver's codebase. Assigning the area is particularly useful for bugs

Projects

None yet

Milestone

Cheops

Development

Successfully merging this pull request may close these issues.

5 participants

@giancarloromeo @wvangeit @pcrespov @odeimaiz @sanderegg