pause or resume the deployment while activating or deactivating the schedules #214
Conversation
…ivating the schedules
WalkthroughAdds a top-level paused field to the Prefect deployment PATCH payload in set_deployment_schedule; paused is true when status == "inactive". The schedule.active flag remains derived from status. No public API/signature changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as Service.set_deployment_schedule
participant Prefect as Prefect API
Client->>Service: set_deployment_schedule(status, schedule)
Service->>Prefect: PATCH /deployments/{id}\n{ schedules: [schedule], paused: (status == "inactive") }
Prefect-->>Service: 200 OK
Service-->>Client: None
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
proxy/service.py (1)
1167-1171: Add minimal tests to prevent regressionsRecommend tests for:
- Pause/resume without schedules (should still PATCH paused).
- Multiple schedules preserved and toggled.
- Invalid status raises ValueError.
I can scaffold tests or open an issue with acceptance criteria.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy/service.py(1 hunks)
🔇 Additional comments (2)
proxy/service.py (2)
1168-1171: Includingpausedin the patch aligns with the PR goalAdding
"paused": status == "inactive"to the deployment patch correctly ties pause/resume to schedule activation/deactivation as requested.
1168-1171: Prefect APIpausedfield confirmed
Verified against the official Prefect 2 REST API (PATCH /api/deployments/{id})—the JSON body uses a booleanpausedfield to pause (true) or resume (false) a deployment. No changes needed.• Location: proxy/service.py, lines 1168–1171
|
|
||
| prefect_patch( | ||
| f"deployments/{deployment_id}", | ||
| {"schedules": [schedule], "paused": status == "inactive"}, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Always patch paused (even when there are no schedules) and update all schedules, not just the first
Current flow skips patching entirely if the deployment has zero schedules, so pause/resume never takes effect in that case. Also, by patching with only the first schedule, you’ll inadvertently drop any additional schedules on the deployment.
Refactor to:
- Validate
status("active" | "inactive"). - Always patch
"paused", regardless of schedules presence. - Update the
activeflag on all schedules and preserve the entire schedules list.
Apply this diff:
def set_deployment_schedule(deployment_id: str, status: str) -> None:
"""Set deployment schedule to active or inactive"""
# both the apis return null below
deployment = prefect_get(f"deployments/{deployment_id}")
- if len(deployment["schedules"]) > 0:
- schedule = deployment["schedules"][0]
- del schedule["id"]
- del schedule["created"]
- del schedule["updated"]
- del schedule["deployment_id"]
- schedule["active"] = status == "active"
-
- prefect_patch(
- f"deployments/{deployment_id}",
- {"schedules": [schedule], "paused": status == "inactive"},
- )
+ allowed = {"active", "inactive"}
+ if status not in allowed:
+ raise ValueError(f"status must be one of {allowed}")
+
+ paused = status == "inactive"
+ payload = {"paused": paused}
+
+ if len(deployment["schedules"]) > 0:
+ new_schedules = []
+ for s in deployment["schedules"]:
+ # Drop server-managed fields before patching
+ s_clean = {
+ k: v
+ for k, v in s.items()
+ if k not in {"id", "created", "updated", "deployment_id"}
+ }
+ s_clean["active"] = not paused
+ new_schedules.append(s_clean)
+ payload["schedules"] = new_schedules
+
+ prefect_patch(f"deployments/{deployment_id}", payload)
return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| prefect_patch( | |
| f"deployments/{deployment_id}", | |
| {"schedules": [schedule], "paused": status == "inactive"}, | |
| ) | |
| def set_deployment_schedule(deployment_id: str, status: str) -> None: | |
| """Set deployment schedule to active or inactive""" | |
| # both the apis return null below | |
| deployment = prefect_get(f"deployments/{deployment_id}") | |
| allowed = {"active", "inactive"} | |
| if status not in allowed: | |
| raise ValueError(f"status must be one of {allowed}") | |
| paused = status == "inactive" | |
| payload = {"paused": paused} | |
| if len(deployment["schedules"]) > 0: | |
| new_schedules = [] | |
| for s in deployment["schedules"]: | |
| # Drop server-managed fields before patching | |
| s_clean = { | |
| k: v | |
| for k, v in s.items() | |
| if k not in {"id", "created", "updated", "deployment_id"} | |
| } | |
| s_clean["active"] = not paused | |
| new_schedules.append(s_clean) | |
| payload["schedules"] = new_schedules | |
| prefect_patch(f"deployments/{deployment_id}", payload) | |
| return None |
…ect airbyte version
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #214 +/- ##
=======================================
Coverage 76.90% 76.90%
=======================================
Files 5 5
Lines 1429 1429
=======================================
Hits 1099 1099
Misses 330 330 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Ishankoradia
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_service.py (1)
1501-1523: Consider parametrizing activation/deactivation tests and add invalid-status coverageBoth tests share identical scaffolding and only vary expected paused/active values. Parametrizing would DRY the test logic and make future changes less error-prone. Also consider adding a negative test for invalid status values to lock in error handling behavior.
Would you like me to provide a pytest-parametrized version covering ("active", paused=False, active=True) and ("inactive", paused=True, active=False), plus an invalid status case?
Also applies to: 1525-1547
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
tests/test_service.py(2 hunks)
🔇 Additional comments (2)
tests/test_service.py (2)
1521-1522: Asserted paused=False on activation — correct and aligned with PR objectiveIncluding the top-level "paused": False when status is "active" matches Prefect’s deployment pause/resume semantics and ensures resuming occurs alongside schedule activation.
1545-1546: Asserted paused=True on deactivation — correct mappingIncluding "paused": True when status is "inactive" correctly pauses the deployment in tandem with disabling the schedule.
Summary by CodeRabbit