[14.0][FIX] cetmix_tower_server_notify_backend On delete flight plan issue #197
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🧹 Nitpick comments (2)
cetmix_tower_server_notify_backend/models/cx_tower_command_log.py (2)
17-19: LGTM! Clear explanation of the edge case.The comments effectively explain the race condition where records might be deleted during Flight Plan execution on server deletion. The
exists()check is the correct approach to handle this scenario.Consider adding a proper type annotation for the
exists()method instead of using# type: ignore. You can use:from typing import cast # ... if not cast(bool, rec.exists()) or not rec.plan_log_id:
Line range hint
21-40: Consider caching required values before notifications.While the existence check prevents initial errors, the record could still be deleted between the check and notification calls. Consider storing required values before sending notifications.
if not rec.exists() or not rec.plan_log_id: # type: ignore continue + +# Cache required values to prevent potential errors if record is deleted +command_name = rec.command_id.name +server_name = rec.server_id.name + if rec.command_status == 0: rec.create_uid.notify_success( message=_( "%(timestamp)s<br/>" "Command '%(name)s' finished successfully", - name=rec.command_id.name, + name=command_name, timestamp=context_timestamp, ), - title=rec.server_id.name, + title=server_name, sticky=True, ) else: rec.create_uid.notify_danger( message=_( "%(timestamp)s<br/>" "Command '%(name)s'" " finished with error. " "Please check the command log for details.", - name=rec.command_id.name, + name=command_name, timestamp=context_timestamp, ), - title=rec.server_id.name, + title=server_name, sticky=True, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cetmix_tower_server_notify_backend/models/cx_tower_command_log.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
ivs-cetmix
force-pushed
the
14.0-t4320-cetmix_tower_server_notify_backend-fix_on_delete
branch
from
a058f45 to
e1fe3a5
Compare
ivs-cetmix
changed the title
When a Server is deleted an on delete Flight Plan is triggered. As soon as this Flight Plan finishes with success, the Server is deleted too. All related records are deleted with it. This creates an issue when notification is sent for a log record that is already deleted. This commit fixes this issue by checking if the record exists before sending a notification. Task: 4320
ivs-cetmix
force-pushed
the
14.0-t4320-cetmix_tower_server_notify_backend-fix_on_delete
branch
from
e1fe3a5 to
f601a58
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cetmix_tower_server_notify_backend/readme/.newsfragments/4320.bugfix (1)
1-1: Enhance the news fragment with more details about the fix.While the error message is clear, consider expanding it to include information about the implemented fix. This helps users understand both the issue and its resolution.
-Error when deleting a Server with an "On Delete Plan" set. +Fixed an issue where notifications were being sent for deleted records when deleting a Server with an "On Delete Plan" set. The system now verifies record existence before sending notifications.cetmix_tower_server_notify_backend/static/description/index.html (1)
381-385: Consider adding changelog entries.The new "Changelog" section has been added but appears to be empty. Consider documenting the changes made in this PR, particularly the fix for the server deletion notification issue.
Also applies to: 398-400
cetmix_tower_server_notify_backend/readme/DESCRIPTION.md (1)
1-3: Fix bare URL in documentation.The documentation link should be formatted as a proper Markdown link to comply with markdownlint guidelines.
Apply this diff to fix the bare URL:
-Please check the official documentation for more information: https://cetmix.com/tower +Please check the official documentation for more information: [https://cetmix.com/tower](https://cetmix.com/tower)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
3-3: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cetmix_tower_server_notify_backend/README.rst(3 hunks)cetmix_tower_server_notify_backend/models/cx_tower_command_log.py(1 hunks)cetmix_tower_server_notify_backend/readme/.newsfragments/4320.bugfix(1 hunks)cetmix_tower_server_notify_backend/readme/DESCRIPTION.md(1 hunks)cetmix_tower_server_notify_backend/static/description/index.html(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- cetmix_tower_server_notify_backend/README.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- cetmix_tower_server_notify_backend/models/cx_tower_command_log.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
cetmix_tower_server_notify_backend/readme/DESCRIPTION.md
3-3: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (2)
cetmix_tower_server_notify_backend/static/description/index.html (2)
372-375: LGTM! Documentation updates align with the PR objectives.The simplified module description and documentation link provide clearer guidance for users.
402-406: LGTM! Updated links and contribution note.The bug tracker links and credits section have been properly updated to reflect version 14.0.
Also applies to: 410-419
|
/ocabot merge patch |
|
On my way to merge this fine PR! |
|
Congratulations, your PR was merged at 4e8191b. Thanks a lot for contributing to cetmix. ❤️ |
CetmixGitDrone
deleted the
14.0-t4320-cetmix_tower_server_notify_backend-fix_on_delete
branch
When a Server is deleted an on delete Flight Plan is triggered. As soon as this Flight Plan finishes with success, the Server is deleted too. All related records are deleted with it.
This creates an issue when notification is sent for a log record that is already deleted.
This commit fixes this issue by checking if the record exists before sending a notification.
Task: 4320
Summary by CodeRabbit
Bug Fixes
Documentation