Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (56)
📝 WalkthroughWalkthroughException handlers across analytics, API, core, activation, service, task, and websocket modules now use ChangesException logging standardization
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
jcraiglo1
marked this pull request as ready for review
July 22, 2026 18:19
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/aap_eda/services/activation/activation_manager.py (1)
858-861: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplicitly chain exceptions when wrapping them.
The affected handlers currently rely on implicit exception context. Add
from e/from exto preserve deliberate cause chains and clear Ruff B904.
src/aap_eda/services/activation/activation_manager.py#L858-L861: raiseActivationMonitorError(...) from e.src/aap_eda/services/activation/engine/podman.py#L422-L431: chain both convertedAPIErrorraises withfrom e.src/aap_eda/services/sync_certs.py#L145-L151: chain connection and timeoutGatewayAPIErrorraises withfrom ex.src/aap_eda/services/sync_certs.py#L202-L208: chain connection and timeoutGatewayAPIErrorraises withfrom ex.src/aap_eda/services/sync_certs.py#L243-L249: chain connection and timeoutGatewayAPIErrorraises withfrom ex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/services/activation/activation_manager.py` around lines 858 - 861, Explicitly chain wrapped exceptions at all affected handlers: in src/aap_eda/services/activation/activation_manager.py lines 858-861, raise ActivationMonitorError from e; in src/aap_eda/services/activation/engine/podman.py lines 422-431, add from e to both converted APIError raises; and in src/aap_eda/services/sync_certs.py lines 145-151, 202-208, and 243-249, add from ex to each connection and timeout GatewayAPIError raise. Preserve the existing error messages and handling.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/aap_eda/services/activation/activation_manager.py`:
- Around line 858-861: Explicitly chain wrapped exceptions at all affected
handlers: in src/aap_eda/services/activation/activation_manager.py lines
858-861, raise ActivationMonitorError from e; in
src/aap_eda/services/activation/engine/podman.py lines 422-431, add from e to
both converted APIError raises; and in src/aap_eda/services/sync_certs.py lines
145-151, 202-208, and 243-249, add from ex to each connection and timeout
GatewayAPIError raise. Preserve the existing error messages and handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: fe204780-a962-457a-b011-367ac8e4088c
📒 Files selected for processing (37)
src/aap_eda/analytics/analytics_collectors.pysrc/aap_eda/analytics/collector.pysrc/aap_eda/analytics/utils.pysrc/aap_eda/api/serializers/activation.pysrc/aap_eda/api/views/activation.pysrc/aap_eda/api/views/credential_type.pysrc/aap_eda/api/views/eda_credential.pysrc/aap_eda/api/views/event_stream.pysrc/aap_eda/api/views/external_event_stream.pysrc/aap_eda/conf/registry.pysrc/aap_eda/core/health.pysrc/aap_eda/core/management/commands/dispatcherctl.pysrc/aap_eda/core/management/commands/dispatcherd.pysrc/aap_eda/core/models/project.pysrc/aap_eda/core/tasking/__init__.pysrc/aap_eda/core/utils/credential_plugins.pysrc/aap_eda/core/utils/credentials.pysrc/aap_eda/core/utils/external_sms.pysrc/aap_eda/core/utils/rulebook.pysrc/aap_eda/services/activation/activation_manager.pysrc/aap_eda/services/activation/drools_cleanup.pysrc/aap_eda/services/activation/engine/kubernetes.pysrc/aap_eda/services/activation/engine/podman.pysrc/aap_eda/services/pg_notify.pysrc/aap_eda/services/sync_certs.pysrc/aap_eda/tasks/orchestrator.pysrc/aap_eda/tasks/project.pysrc/aap_eda/utils/__init__.pysrc/aap_eda/wsapi/consumers.pytests/integration/analytics/test_collector.pytests/integration/analytics/test_utils.pytests/integration/core/test_dispatcherd_real.pytests/integration/management/test_dispatcherd_commands.pytests/integration/services/activation/test_drools_cleanup.pytests/integration/tasks/test_project_dispatcherd.pytests/integration/tasks/test_projects.pytests/unit/test_orchestrator.py
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1626 +/- ##
==========================================
+ Coverage 93.18% 94.13% +0.94%
==========================================
Files 244 244
Lines 11376 11376
==========================================
+ Hits 10601 10709 +108
+ Misses 775 667 -108
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
|
One comment that applies to all of this PR: except DatabaseError as e:
logger.exception(f"Database error occurred: {e}")The Hence the explicit printing of the caught exception is redundant, and should be dropped. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/aap_eda/services/activation/activation_manager.py (2)
858-861: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve the explicit exception cause when re-raising.
Change
raise exceptions.ActivationMonitorError(f"{e}")toraise ... from e; otherwise the wrapper does not explicitly preserve the caught exception as its cause.As per path instructions, this review focuses on major issues impacting performance, readability, maintainability and security and avoids nitpicks and verbosity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/services/activation/activation_manager.py` around lines 858 - 861, Update the exception handling in the activation monitor operation around ActivationInstanceNotFound so the raised ActivationMonitorError explicitly chains the caught exception e as its cause. Preserve the existing error message and logging behavior while changing only the re-raise in this except block.Sources: Path instructions, Linters/SAST tools
294-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid interpolating caught exceptions into
LOGGER.exceptionmessages.These messages already contain
Reason: {exc}, so the exception text is logged twice alongside the traceback. Log static context withLOGGER.exception(...); retain the detailedmsgseparately where it is needed for status or user-facing output.As per path instructions, this review focuses on major issues impacting performance, readability, maintainability and security and avoids nitpicks and verbosity.
Also applies to: 433-438, 540-545, 575-580, 607-612, 789-794, 802-807, 1049-1055
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/services/activation/activation_manager.py` around lines 294 - 302, Update the exception handlers in the activation manager, including the cleanup failure flow and the additional indicated handlers, so LOGGER.exception receives only static contextual text without interpolating the caught exception. Preserve each detailed msg containing the exception for log_handler.write or other status/user-facing output that still requires it.Source: Path instructions
src/aap_eda/api/views/event_stream.py (1)
369-381: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve causes when wrapping exceptions and use explicit f-string conversions.
Use
f"{ex!s}"instead off"{str(ex)}"and raise replacement exceptions withfrom ex. This satisfies the Ruff findings and makes the exception relationship explicit.
src/aap_eda/api/views/event_stream.py#L369-L381: applyfrom exand!sto both API exception wrappers.src/aap_eda/services/sync_certs.py#L145-L151: apply them to_make_requestwrappers.src/aap_eda/services/sync_certs.py#L198-L205: apply them to_delete_from_gatewaywrappers.src/aap_eda/services/sync_certs.py#L235-L242: apply them to_fetch_from_gatewaywrappers.As per path instructions, this review focuses on major issues impacting performance, readability, maintainability and security and avoids nitpicks and verbosity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/api/views/event_stream.py` around lines 369 - 381, Update both API exception wrappers in src/aap_eda/api/views/event_stream.py at lines 369-381 to use explicit f-string string conversion (!s) and raise the replacement exceptions from ex. Apply the same !s conversion and from ex chaining to the exception wrappers around _make_request at src/aap_eda/services/sync_certs.py lines 145-151, _delete_from_gateway at lines 198-205, and _fetch_from_gateway at lines 235-242.Sources: Path instructions, Linters/SAST tools
tests/integration/api/test_activation.py (1)
1627-1629: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared helper for the repeated
logger.exceptionassertion idiom. The same "assert called once + check substring incall_args[0][0]" pattern is copy-pasted across 5 files for this PR's new logging-coverage tests. A small shared helper would reduce duplication as more of these tests are added.
tests/integration/api/test_activation.py#L1627-L1629: replace with a call to a sharedassert_logger_exception(mock_logger, "Failed to start project sync")helper.tests/integration/api/test_activation.py#L2958-L2960: same helper, substring"Failed to update event stream source".tests/integration/api/test_credential_type.py#L1304-L1305: same helper, substring"Plugin call failed".tests/integration/api/test_eda_credential.py#L2862-L2863: same helper, substring"call failed".tests/integration/core/test_rulebook.py#L355-L356: same helper, substring"Invalid rulesets".tests/integration/services/test_sync_certs.py#L799-L801: same helper, substring"Connection error".tests/integration/services/test_sync_certs.py#L821-L823: same helper, substring"Timeout".tests/integration/services/test_sync_certs.py#L850-L852: same helper, substring"gateway certificate".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/api/test_activation.py` around lines 1627 - 1629, Repeated logger.exception assertions should use one shared assert_logger_exception helper. Add or reuse that helper to assert the logger was called once and that its message contains the expected substring, then replace the duplicated assertions in tests/integration/api/test_activation.py lines 1627-1629 and 2958-2960, tests/integration/api/test_credential_type.py lines 1304-1305, tests/integration/api/test_eda_credential.py lines 2862-2863, tests/integration/core/test_rulebook.py lines 355-356, and tests/integration/services/test_sync_certs.py lines 799-801, 821-823, and 850-852 with the helper using their existing expected messages.src/aap_eda/services/activation/engine/podman.py (1)
208-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove caught-exception interpolation from
logger.exceptioncalls.
logger.exceptionrecords the caught exception and traceback, so embeddinge/str(e)in those messages is redundant and can duplicate log output. Keep useful context such as project/container IDs orerror_messagefor the raised user-facing exception.
src/aap_eda/services/activation/engine/podman.py#L214: use the static container-error message;error_messageis already written to container logs and re-raised.src/aap_eda/services/activation/engine/podman.py#L419/L434: log image pull context withoute;reqest.image_url/msgalready provide the useful context.src/aap_eda/tasks/project.py#L98/L103/L108/L169/L174/L179: log project context only; the exception is captured in the traceback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/services/activation/engine/podman.py` around lines 208 - 216, Remove caught-exception interpolation from logger.exception calls: in src/aap_eda/services/activation/engine/podman.py lines 208-216, log the static container-start context while retaining error_message for container logs and ContainerStartError; in lines 417-436, retain only image-pull context such as reqest.image_url or msg; and in src/aap_eda/tasks/project.py lines 97-110 and 168-181, log project context without e or str(e), relying on logger.exception for traceback details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/services/activation/engine/test_kubernetes.py`:
- Around line 1247-1262: Update
test_set_namespace_logs_exception_on_missing_file to explicitly remove
EDA_ACTIVATION_JOB_NAMESPACE from the environment and mock open to raise
FileNotFoundError during Engine initialization. Keep the existing
ContainerEngineInitError assertion and LOGGER mock so the test deterministically
exercises the missing service-account file exception path.
---
Nitpick comments:
In `@src/aap_eda/api/views/event_stream.py`:
- Around line 369-381: Update both API exception wrappers in
src/aap_eda/api/views/event_stream.py at lines 369-381 to use explicit f-string
string conversion (!s) and raise the replacement exceptions from ex. Apply the
same !s conversion and from ex chaining to the exception wrappers around
_make_request at src/aap_eda/services/sync_certs.py lines 145-151,
_delete_from_gateway at lines 198-205, and _fetch_from_gateway at lines 235-242.
In `@src/aap_eda/services/activation/activation_manager.py`:
- Around line 858-861: Update the exception handling in the activation monitor
operation around ActivationInstanceNotFound so the raised ActivationMonitorError
explicitly chains the caught exception e as its cause. Preserve the existing
error message and logging behavior while changing only the re-raise in this
except block.
- Around line 294-302: Update the exception handlers in the activation manager,
including the cleanup failure flow and the additional indicated handlers, so
LOGGER.exception receives only static contextual text without interpolating the
caught exception. Preserve each detailed msg containing the exception for
log_handler.write or other status/user-facing output that still requires it.
In `@src/aap_eda/services/activation/engine/podman.py`:
- Around line 208-216: Remove caught-exception interpolation from
logger.exception calls: in src/aap_eda/services/activation/engine/podman.py
lines 208-216, log the static container-start context while retaining
error_message for container logs and ContainerStartError; in lines 417-436,
retain only image-pull context such as reqest.image_url or msg; and in
src/aap_eda/tasks/project.py lines 97-110 and 168-181, log project context
without e or str(e), relying on logger.exception for traceback details.
In `@tests/integration/api/test_activation.py`:
- Around line 1627-1629: Repeated logger.exception assertions should use one
shared assert_logger_exception helper. Add or reuse that helper to assert the
logger was called once and that its message contains the expected substring,
then replace the duplicated assertions in
tests/integration/api/test_activation.py lines 1627-1629 and 2958-2960,
tests/integration/api/test_credential_type.py lines 1304-1305,
tests/integration/api/test_eda_credential.py lines 2862-2863,
tests/integration/core/test_rulebook.py lines 355-356, and
tests/integration/services/test_sync_certs.py lines 799-801, 821-823, and
850-852 with the helper using their existing expected messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b15868c8-6143-4387-9cf7-8ec6c1e5d668
📒 Files selected for processing (35)
src/aap_eda/analytics/analytics_collectors.pysrc/aap_eda/analytics/collector.pysrc/aap_eda/analytics/utils.pysrc/aap_eda/api/serializers/activation.pysrc/aap_eda/api/views/event_stream.pysrc/aap_eda/core/utils/credentials.pysrc/aap_eda/core/utils/rulebook.pysrc/aap_eda/services/activation/activation_manager.pysrc/aap_eda/services/activation/engine/podman.pysrc/aap_eda/services/activation/engine/ports.pysrc/aap_eda/services/pg_notify.pysrc/aap_eda/services/sync_certs.pysrc/aap_eda/tasks/orchestrator.pysrc/aap_eda/tasks/project.pysrc/aap_eda/wsapi/consumers.pytests/integration/analytics/test_analytics_collectors.pytests/integration/analytics/test_collector.pytests/integration/analytics/test_utils.pytests/integration/api/test_activation.pytests/integration/api/test_credential_type.pytests/integration/api/test_eda_credential.pytests/integration/api/test_event_stream.pytests/integration/api/test_event_stream_basic.pytests/integration/api/test_project.pytests/integration/core/test_health.pytests/integration/core/test_rulebook.pytests/integration/services/activation/engine/test_kubernetes.pytests/integration/services/activation/engine/test_podman.pytests/integration/services/activation/test_manager.pytests/integration/services/test_sync_certs.pytests/integration/wsapi/test_consumer.pytests/unit/services/test_pg_notify.pytests/unit/test_credential_plugins.pytests/unit/test_credential_validation.pytests/unit/test_external_sms.py
🚧 Files skipped from review as they are similar to previous changes (11)
- src/aap_eda/services/pg_notify.py
- tests/integration/analytics/test_collector.py
- src/aap_eda/analytics/collector.py
- src/aap_eda/core/utils/rulebook.py
- src/aap_eda/api/serializers/activation.py
- src/aap_eda/analytics/analytics_collectors.py
- src/aap_eda/wsapi/consumers.py
- src/aap_eda/core/utils/credentials.py
- src/aap_eda/tasks/orchestrator.py
- src/aap_eda/analytics/utils.py
- tests/integration/analytics/test_utils.py
Comment on lines
+1247
to
+1262
| @mock.patch( | ||
| "aap_eda.services.activation.engine.kubernetes.LOGGER", | ||
| ) | ||
| @pytest.mark.django_db | ||
| def test_set_namespace_logs_exception_on_missing_file( | ||
| mock_logger, | ||
| init_kubernetes_data, | ||
| ): | ||
| """_set_namespace calls LOGGER.exception when file is missing.""" | ||
| activation_id = init_kubernetes_data.activation.id | ||
| with pytest.raises(ContainerEngineInitError): | ||
| Engine( | ||
| activation_id=str(activation_id), | ||
| resource_prefix=ProcessParentType.ACTIVATION, | ||
| client=mock.Mock(), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the missing-file setup deterministic.
This test only reaches the intended branch when EDA_ACTIVATION_JOB_NAMESPACE is unset and the service-account file is absent. Clear the environment variable and mock open to raise FileNotFoundError; otherwise the test can fail or silently stop covering the exception path in a Kubernetes-configured runner.
Proposed fix
def test_set_namespace_logs_exception_on_missing_file(
mock_logger,
init_kubernetes_data,
+ monkeypatch,
):
"""_set_namespace calls LOGGER.exception when file is missing."""
+ monkeypatch.delenv("EDA_ACTIVATION_JOB_NAMESPACE", raising=False)
activation_id = init_kubernetes_data.activation.id
- with pytest.raises(ContainerEngineInitError):
- Engine(
- activation_id=str(activation_id),
- resource_prefix=ProcessParentType.ACTIVATION,
- client=mock.Mock(),
- )
+ with mock.patch("builtins.open", side_effect=FileNotFoundError):
+ with pytest.raises(ContainerEngineInitError):
+ Engine(
+ activation_id=str(activation_id),
+ resource_prefix=ProcessParentType.ACTIVATION,
+ client=mock.Mock(),
+ )📝 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.
Suggested change
| @mock.patch( | |
| "aap_eda.services.activation.engine.kubernetes.LOGGER", | |
| ) | |
| @pytest.mark.django_db | |
| def test_set_namespace_logs_exception_on_missing_file( | |
| mock_logger, | |
| init_kubernetes_data, | |
| ): | |
| """_set_namespace calls LOGGER.exception when file is missing.""" | |
| activation_id = init_kubernetes_data.activation.id | |
| with pytest.raises(ContainerEngineInitError): | |
| Engine( | |
| activation_id=str(activation_id), | |
| resource_prefix=ProcessParentType.ACTIVATION, | |
| client=mock.Mock(), | |
| ) | |
| `@mock.patch`( | |
| "aap_eda.services.activation.engine.kubernetes.LOGGER", | |
| ) | |
| `@pytest.mark.django_db` | |
| def test_set_namespace_logs_exception_on_missing_file( | |
| mock_logger, | |
| init_kubernetes_data, | |
| monkeypatch, | |
| ): | |
| """_set_namespace calls LOGGER.exception when file is missing.""" | |
| monkeypatch.delenv("EDA_ACTIVATION_JOB_NAMESPACE", raising=False) | |
| activation_id = init_kubernetes_data.activation.id | |
| with mock.patch("builtins.open", side_effect=FileNotFoundError): | |
| with pytest.raises(ContainerEngineInitError): | |
| Engine( | |
| activation_id=str(activation_id), | |
| resource_prefix=ProcessParentType.ACTIVATION, | |
| client=mock.Mock(), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/services/activation/engine/test_kubernetes.py` around lines
1247 - 1262, Update test_set_namespace_logs_exception_on_missing_file to
explicitly remove EDA_ACTIVATION_JOB_NAMESPACE from the environment and mock
open to raise FileNotFoundError during Engine initialization. Keep the existing
ContainerEngineInitError assertion and LOGGER mock so the test deterministically
exercises the missing service-account file exception path.
jcraiglo1
force-pushed
the
sonar/S8572-logging-exception
branch
from
July 23, 2026 19:56
9edbf85 to
d1ec201
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/aap_eda/api/views/event_stream.py (2)
372-372: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit conversion flags instead of nested
str()calls.
src/aap_eda/api/views/event_stream.py#L372-L372: replacestr(ex)withex!s.src/aap_eda/tasks/project.py#L101-L101: replacestr(e)withe!s.src/aap_eda/tasks/project.py#L172-L172: replacestr(e)withe!s.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/api/views/event_stream.py` at line 372, Replace the nested string conversions in the f-strings with explicit conversion flags: update ex in event_stream.py at lines 372-372, and e in project.py at lines 101-101 and 172-172, using the string conversion flag while preserving the surrounding messages.Source: Linters/SAST tools
370-373: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExplicitly preserve translated exception causes.
Both changed exception translations omit explicit
fromchaining.
src/aap_eda/api/views/event_stream.py#L370-L373: raiseGatewayAPIErrorwithfrom ex.src/aap_eda/services/activation/engine/podman.py#L422-L431: raise both pull-error wrappers withfrom e.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/api/views/event_stream.py` around lines 370 - 373, Preserve explicit exception chaining in both translation sites: update the GatewayAPIError raised in event_stream.py lines 370-373 to chain from ex, and update both pull-error wrapper raises in podman.py lines 422-431 to chain from e. Do not alter the existing error messages or handling behavior.Source: Linters/SAST tools
src/aap_eda/services/activation/engine/podman.py (1)
213-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLet
logger.exceptionprovide the exception text and traceback. The caught exception is redundantly interpolated into each logger message; retain it only in user-facing/recovery messages and pass static or parameterized context to the logger.
src/aap_eda/services/activation/engine/podman.py#L213-L216: log"Container Start Error"separately fromerror_message.src/aap_eda/services/activation/engine/podman.py#L432-L436: log timeout context separately from themsgused for propagation.src/aap_eda/tasks/project.py#L98-L109: remove{e}from import exception logger messages.src/aap_eda/tasks/project.py#L169-L180: remove{e}from sync exception logger messages.src/aap_eda/tasks/project.py#L202-L212: remove{e}from post-sync exception logger messages.src/aap_eda/tasks/project.py#L241-L246: remove{e}from auto-restart check logger messages.src/aap_eda/tasks/project.py#L327-L341: remove{e}and{save_err}from restart logger messages.src/aap_eda/tasks/project.py#L426-L446: remove{e}and{save_err}from resume logger messages.src/aap_eda/tasks/project.py#L476-L485: remove{e}from sync-failure logger messages.src/aap_eda/tasks/project.py#L582-L585: remove{e}from stuck-project recovery logger messages.src/aap_eda/tasks/project.py#L646-L650: remove{e}from orphaned-activation recovery logger messages.As per path instructions: “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/services/activation/engine/podman.py` around lines 213 - 216, Separate exception logging context from propagated or user-facing error messages: in src/aap_eda/services/activation/engine/podman.py lines 213-216, keep error_message for log_handler and ContainerStartError but pass static “Container Start Error” context to LOGGER.exception; in lines 432-436, log static timeout context separately from msg. In src/aap_eda/tasks/project.py lines 98-109, 169-180, 202-212, 241-246, 476-485, 582-585, and 646-650, remove exception interpolation from logger messages; in lines 327-341 and 426-446, remove both exception and save-error interpolation, while retaining those values for propagation or user-facing recovery messages. Let logger.exception supply exception text and traceback at every listed site.Source: Path instructions
src/aap_eda/services/activation/activation_manager.py (2)
859-861: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplicitly chain the caught monitoring exception.
This
except ... as ebranch raisesActivationMonitorErrorwithoutfrom e, which triggers Ruff B904 and obscures the explicit causal relationship. Raise it withfrom e.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/services/activation/activation_manager.py` around lines 859 - 861, Update the exception raise in the monitoring error handler to explicitly chain the caught exception by raising ActivationMonitorError from e, while preserving the existing logging and _error_activation call.Source: Linters/SAST tools
301-301: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse context-only messages with
logger.exceptionthroughout the changed handlers.The active exception and traceback are already emitted by
logger.exception; embeddinge/excin the message duplicates the exception text. Preserve detailed strings where they are needed for user-facing status or command errors, but separate the logging message.
src/aap_eda/services/activation/activation_manager.py#L301-L301: log activation/instance context without interpolatingexc.src/aap_eda/services/activation/activation_manager.py#L438-L438: log activation cleanup context without interpolatingexc.src/aap_eda/services/activation/activation_manager.py#L545-L545: log the “restart policy not applicable” cleanup context without interpolatingexc.src/aap_eda/services/activation/activation_manager.py#L580-L580: log the max-restarts cleanup context without interpolatingexc.src/aap_eda/services/activation/activation_manager.py#L612-L612: log restart-path cleanup context without interpolatingexc.src/aap_eda/services/activation/activation_manager.py#L794-L794: log deletion cleanup context without interpolatingexc.src/aap_eda/services/activation/activation_manager.py#L1054-L1054: log update-logs context without interpolatingexc.src/aap_eda/api/views/activation.py#L715-L717: retainefor response handling but remove it from the exception log message.src/aap_eda/core/management/commands/dispatcherctl.py#L142-L142: retainerror_msgforCommandErrorbut log only stable command context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/services/activation/activation_manager.py` at line 301, Update the logger.exception calls in src/aap_eda/services/activation/activation_manager.py at lines 301, 438, 545, 580, 612, 794, and 1054 to use only activation, cleanup, restart, deletion, or update-logs context without interpolating exc. In src/aap_eda/api/views/activation.py lines 715-717, retain e for response handling but remove it from the exception log message; in src/aap_eda/core/management/commands/dispatcherctl.py line 142, retain error_msg for CommandError while logging only stable command context.src/aap_eda/services/activation/engine/kubernetes.py (1)
142-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid duplicating the caught exception in the exception log.
msgalready includesReason: {e}, whileLOGGER.exceptionappends the same exception and traceback. Keep a reason-bearing message forlog_handler.write()if needed, but pass a contextual message without{e}toLOGGER.exception.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/services/activation/engine/kubernetes.py` at line 142, Update the exception-handling block around LOGGER.exception so its argument is a contextual message that does not include the caught exception, while preserving the reason-bearing msg for log_handler.write() if needed. Keep LOGGER.exception responsible for attaching the exception and traceback without duplicating it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aap_eda/tasks/project.py`:
- Line 665: Update the ObjectDoesNotExist handling in _get_project_safely to log
the expected missing-project condition with logger.warning or logger.info
instead of logger.exception, omitting traceback and exception details while
preserving the existing None return behavior.
---
Nitpick comments:
In `@src/aap_eda/api/views/event_stream.py`:
- Line 372: Replace the nested string conversions in the f-strings with explicit
conversion flags: update ex in event_stream.py at lines 372-372, and e in
project.py at lines 101-101 and 172-172, using the string conversion flag while
preserving the surrounding messages.
- Around line 370-373: Preserve explicit exception chaining in both translation
sites: update the GatewayAPIError raised in event_stream.py lines 370-373 to
chain from ex, and update both pull-error wrapper raises in podman.py lines
422-431 to chain from e. Do not alter the existing error messages or handling
behavior.
In `@src/aap_eda/services/activation/activation_manager.py`:
- Around line 859-861: Update the exception raise in the monitoring error
handler to explicitly chain the caught exception by raising
ActivationMonitorError from e, while preserving the existing logging and
_error_activation call.
- Line 301: Update the logger.exception calls in
src/aap_eda/services/activation/activation_manager.py at lines 301, 438, 545,
580, 612, 794, and 1054 to use only activation, cleanup, restart, deletion, or
update-logs context without interpolating exc. In
src/aap_eda/api/views/activation.py lines 715-717, retain e for response
handling but remove it from the exception log message; in
src/aap_eda/core/management/commands/dispatcherctl.py line 142, retain error_msg
for CommandError while logging only stable command context.
In `@src/aap_eda/services/activation/engine/kubernetes.py`:
- Line 142: Update the exception-handling block around LOGGER.exception so its
argument is a contextual message that does not include the caught exception,
while preserving the reason-bearing msg for log_handler.write() if needed. Keep
LOGGER.exception responsible for attaching the exception and traceback without
duplicating it.
In `@src/aap_eda/services/activation/engine/podman.py`:
- Around line 213-216: Separate exception logging context from propagated or
user-facing error messages: in src/aap_eda/services/activation/engine/podman.py
lines 213-216, keep error_message for log_handler and ContainerStartError but
pass static “Container Start Error” context to LOGGER.exception; in lines
432-436, log static timeout context separately from msg. In
src/aap_eda/tasks/project.py lines 98-109, 169-180, 202-212, 241-246, 476-485,
582-585, and 646-650, remove exception interpolation from logger messages; in
lines 327-341 and 426-446, remove both exception and save-error interpolation,
while retaining those values for propagation or user-facing recovery messages.
Let logger.exception supply exception text and traceback at every listed site.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c5385f2-d646-4f24-9081-d297189a2e77
📒 Files selected for processing (56)
src/aap_eda/analytics/analytics_collectors.pysrc/aap_eda/analytics/collector.pysrc/aap_eda/analytics/utils.pysrc/aap_eda/api/serializers/activation.pysrc/aap_eda/api/views/activation.pysrc/aap_eda/api/views/credential_type.pysrc/aap_eda/api/views/eda_credential.pysrc/aap_eda/api/views/event_stream.pysrc/aap_eda/api/views/external_event_stream.pysrc/aap_eda/conf/registry.pysrc/aap_eda/core/health.pysrc/aap_eda/core/management/commands/dispatcherctl.pysrc/aap_eda/core/management/commands/dispatcherd.pysrc/aap_eda/core/models/project.pysrc/aap_eda/core/tasking/__init__.pysrc/aap_eda/core/utils/credential_plugins.pysrc/aap_eda/core/utils/credentials.pysrc/aap_eda/core/utils/external_sms.pysrc/aap_eda/core/utils/rulebook.pysrc/aap_eda/services/activation/activation_manager.pysrc/aap_eda/services/activation/drools_cleanup.pysrc/aap_eda/services/activation/engine/kubernetes.pysrc/aap_eda/services/activation/engine/podman.pysrc/aap_eda/services/activation/engine/ports.pysrc/aap_eda/services/pg_notify.pysrc/aap_eda/services/sync_certs.pysrc/aap_eda/tasks/orchestrator.pysrc/aap_eda/tasks/project.pysrc/aap_eda/utils/__init__.pysrc/aap_eda/wsapi/consumers.pytests/integration/analytics/test_analytics_collectors.pytests/integration/analytics/test_collector.pytests/integration/analytics/test_utils.pytests/integration/api/test_activation.pytests/integration/api/test_credential_type.pytests/integration/api/test_eda_credential.pytests/integration/api/test_event_stream.pytests/integration/api/test_event_stream_basic.pytests/integration/api/test_project.pytests/integration/core/test_dispatcherd_real.pytests/integration/core/test_health.pytests/integration/core/test_rulebook.pytests/integration/management/test_dispatcherd_commands.pytests/integration/services/activation/engine/test_kubernetes.pytests/integration/services/activation/engine/test_podman.pytests/integration/services/activation/test_drools_cleanup.pytests/integration/services/activation/test_manager.pytests/integration/services/test_sync_certs.pytests/integration/tasks/test_project_dispatcherd.pytests/integration/tasks/test_projects.pytests/integration/wsapi/test_consumer.pytests/unit/services/test_pg_notify.pytests/unit/test_credential_plugins.pytests/unit/test_credential_validation.pytests/unit/test_external_sms.pytests/unit/test_orchestrator.py
🚧 Files skipped from review as they are similar to previous changes (40)
- src/aap_eda/services/pg_notify.py
- src/aap_eda/api/views/credential_type.py
- src/aap_eda/core/health.py
- src/aap_eda/core/management/commands/dispatcherd.py
- src/aap_eda/conf/registry.py
- src/aap_eda/core/utils/credential_plugins.py
- src/aap_eda/utils/init.py
- tests/integration/wsapi/test_consumer.py
- tests/integration/tasks/test_project_dispatcherd.py
- src/aap_eda/core/utils/rulebook.py
- src/aap_eda/core/utils/external_sms.py
- src/aap_eda/services/activation/engine/ports.py
- tests/integration/analytics/test_analytics_collectors.py
- src/aap_eda/analytics/collector.py
- tests/integration/management/test_dispatcherd_commands.py
- tests/integration/analytics/test_collector.py
- src/aap_eda/services/activation/drools_cleanup.py
- tests/integration/services/activation/engine/test_kubernetes.py
- src/aap_eda/core/tasking/init.py
- tests/integration/services/activation/test_drools_cleanup.py
- src/aap_eda/analytics/analytics_collectors.py
- src/aap_eda/wsapi/consumers.py
- src/aap_eda/api/serializers/activation.py
- tests/unit/test_external_sms.py
- tests/unit/services/test_pg_notify.py
- src/aap_eda/core/models/project.py
- src/aap_eda/api/views/external_event_stream.py
- tests/integration/api/test_event_stream.py
- src/aap_eda/api/views/eda_credential.py
- tests/unit/test_credential_validation.py
- tests/integration/core/test_dispatcherd_real.py
- tests/integration/api/test_event_stream_basic.py
- tests/integration/api/test_project.py
- src/aap_eda/core/utils/credentials.py
- tests/integration/api/test_eda_credential.py
- tests/integration/api/test_activation.py
- tests/integration/services/activation/engine/test_podman.py
- src/aap_eda/analytics/utils.py
- tests/integration/core/test_health.py
- tests/integration/analytics/test_utils.py
| return models.Project.objects.get(pk=project_id) | ||
| except ObjectDoesNotExist: | ||
| logger.error(f"Project {project_id} does not exist or was deleted") | ||
| logger.exception(f"Project {project_id} does not exist or was deleted") |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Avoid traceback logging for the expected missing-project path.
_get_project_safely explicitly treats ObjectDoesNotExist as normal and returns None. Logging this at error level with a traceback can flood error logs and alerts during deletion races; use logger.warning or logger.info without exception information.
As per path instructions: “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aap_eda/tasks/project.py` at line 665, Update the ObjectDoesNotExist
handling in _get_project_safely to log the expected missing-project condition
with logger.warning or logger.info instead of logger.exception, omitting
traceback and exception details while preserving the existing None return
behavior.
Source: Path instructions
jcraiglo1
enabled auto-merge (squash)
July 23, 2026 20:12
jcraiglo1
force-pushed
the
sonar/S8572-logging-exception
branch
from
July 23, 2026 20:19
d1ec201 to
1627720
Compare
Contributor
|
Thinking more about this.
I believe Sonar Cloud is pushing towards something that we do not want to. logging.error(f"issue: {e}")Code like the above will log the text string with the exception as its string representation, whereas logging.exception("issue")will log also the whole stack trace. While it can be useful, I do not think we want this for every exception handler whose result is an error. Few of them already use |
jcraiglo1
force-pushed
the
sonar/S8572-logging-exception
branch
from
July 24, 2026 14:35
1627720 to
cb026ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aap_eda/services/activation/activation_manager.py`:
- Around line 728-731: Replace LOGGER.exception with LOGGER.warning or
LOGGER.info in the recovered stop-operation branches, including the
corresponding branch near the other referenced location, so missing instance or
pod races log without exception traceback data while preserving the existing
return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 278d5326-9187-4048-b659-564a8026ccc8
📒 Files selected for processing (56)
src/aap_eda/analytics/analytics_collectors.pysrc/aap_eda/analytics/collector.pysrc/aap_eda/analytics/utils.pysrc/aap_eda/api/serializers/activation.pysrc/aap_eda/api/views/activation.pysrc/aap_eda/api/views/credential_type.pysrc/aap_eda/api/views/eda_credential.pysrc/aap_eda/api/views/event_stream.pysrc/aap_eda/api/views/external_event_stream.pysrc/aap_eda/conf/registry.pysrc/aap_eda/core/health.pysrc/aap_eda/core/management/commands/dispatcherctl.pysrc/aap_eda/core/management/commands/dispatcherd.pysrc/aap_eda/core/models/project.pysrc/aap_eda/core/tasking/__init__.pysrc/aap_eda/core/utils/credential_plugins.pysrc/aap_eda/core/utils/credentials.pysrc/aap_eda/core/utils/external_sms.pysrc/aap_eda/core/utils/rulebook.pysrc/aap_eda/services/activation/activation_manager.pysrc/aap_eda/services/activation/drools_cleanup.pysrc/aap_eda/services/activation/engine/kubernetes.pysrc/aap_eda/services/activation/engine/podman.pysrc/aap_eda/services/activation/engine/ports.pysrc/aap_eda/services/pg_notify.pysrc/aap_eda/services/sync_certs.pysrc/aap_eda/tasks/orchestrator.pysrc/aap_eda/tasks/project.pysrc/aap_eda/utils/__init__.pysrc/aap_eda/wsapi/consumers.pytests/integration/analytics/test_analytics_collectors.pytests/integration/analytics/test_collector.pytests/integration/analytics/test_utils.pytests/integration/api/test_activation.pytests/integration/api/test_credential_type.pytests/integration/api/test_eda_credential.pytests/integration/api/test_event_stream.pytests/integration/api/test_event_stream_basic.pytests/integration/api/test_project.pytests/integration/core/test_dispatcherd_real.pytests/integration/core/test_health.pytests/integration/core/test_rulebook.pytests/integration/management/test_dispatcherd_commands.pytests/integration/services/activation/engine/test_kubernetes.pytests/integration/services/activation/engine/test_podman.pytests/integration/services/activation/test_drools_cleanup.pytests/integration/services/activation/test_manager.pytests/integration/services/test_sync_certs.pytests/integration/tasks/test_project_dispatcherd.pytests/integration/tasks/test_projects.pytests/integration/wsapi/test_consumer.pytests/unit/services/test_pg_notify.pytests/unit/test_credential_plugins.pytests/unit/test_credential_validation.pytests/unit/test_external_sms.pytests/unit/test_orchestrator.py
🚧 Files skipped from review as they are similar to previous changes (40)
- src/aap_eda/core/management/commands/dispatcherctl.py
- src/aap_eda/core/management/commands/dispatcherd.py
- src/aap_eda/api/views/credential_type.py
- tests/unit/test_credential_plugins.py
- src/aap_eda/conf/registry.py
- src/aap_eda/services/activation/engine/ports.py
- src/aap_eda/utils/init.py
- tests/integration/analytics/test_analytics_collectors.py
- tests/integration/core/test_rulebook.py
- tests/integration/services/activation/test_drools_cleanup.py
- src/aap_eda/core/utils/rulebook.py
- tests/unit/test_external_sms.py
- tests/integration/management/test_dispatcherd_commands.py
- src/aap_eda/core/health.py
- src/aap_eda/core/utils/external_sms.py
- src/aap_eda/core/models/project.py
- src/aap_eda/analytics/collector.py
- src/aap_eda/services/activation/engine/kubernetes.py
- tests/integration/api/test_project.py
- src/aap_eda/core/utils/credential_plugins.py
- src/aap_eda/analytics/analytics_collectors.py
- src/aap_eda/api/views/activation.py
- tests/integration/wsapi/test_consumer.py
- tests/integration/api/test_credential_type.py
- tests/integration/api/test_eda_credential.py
- tests/unit/services/test_pg_notify.py
- src/aap_eda/api/views/external_event_stream.py
- tests/integration/api/test_event_stream.py
- tests/integration/analytics/test_utils.py
- src/aap_eda/core/tasking/init.py
- tests/integration/services/activation/engine/test_kubernetes.py
- tests/unit/test_credential_validation.py
- tests/integration/api/test_activation.py
- tests/integration/core/test_dispatcherd_real.py
- src/aap_eda/analytics/utils.py
- src/aap_eda/core/utils/credentials.py
- tests/integration/api/test_event_stream_basic.py
- tests/integration/core/test_health.py
- tests/integration/services/activation/test_manager.py
- tests/integration/tasks/test_projects.py
Comment on lines
+728
to
731
| LOGGER.exception( | ||
| f"Stop operation activation id: {self.db_instance.id} " | ||
| "No instance found.", | ||
| ) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid tracebacks for recovered lifecycle states.
Both branches deliberately recover and return, but LOGGER.exception turns routine missing-instance/pod races into error tracebacks. Use LOGGER.warning or LOGGER.info without exception info to avoid noisy logs and alerts.
As per path instructions, “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”
Also applies to: 1035-1038
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aap_eda/services/activation/activation_manager.py` around lines 728 -
731, Replace LOGGER.exception with LOGGER.warning or LOGGER.info in the
recovered stop-operation branches, including the corresponding branch near the
other referenced location, so missing instance or pod races log without
exception traceback data while preserving the existing return behavior.
Source: Path instructions
Contributor
Author
|
@ptoscano I agree that this is maybe a step too far for sonar. The rule states that logger.exception should be used in exception handlers but tbh I think this is a bit pedantic. I can raise this in the sonarcloud channel if you feel this should be reviewed? Edit: https://sonarcloud.io/organizations/ansible/rules?open=python%3AS8572&rule_key=python%3AS8572 this is the rule stating it and there's not a why listed here |
Contributor
Yes, please.
Yeah, sadly a good amount of Sonar Cloud rules as like this, with no additional explanation/reason... |
…traceback Signed-off-by: Jacob Craiglow <jacobcraiglow1@gmail.com>
…traceback Signed-off-by: Jacob Craiglow <jacobcraiglow1@gmail.com>
jcraiglo1
force-pushed
the
sonar/S8572-logging-exception
branch
from
July 27, 2026 18:29
cb026ba to
68ebe0d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/aap_eda/api/views/eda_credential.py (1)
328-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid duplicating exception text in
logger.exceptionacross these handlers.Each site passes a message containing the caught exception to
logger.exception, duplicating text already included by the traceback. Keep detailed messages for outward-facing raised errors, but use separate log templates without the exception interpolation.
src/aap_eda/api/views/eda_credential.py#L328-L332: remove the finalerrformat argument and unused binding if no longer needed.src/aap_eda/core/management/commands/dispatcherctl.py#L141-L142: keeperror_msgforCommandError, but log a template containing only the command.src/aap_eda/core/utils/external_sms.py#L50-L50: log a separate field/context message while preservingmsgfor the re-raised error.As noted in the PR comments summary, explicit exception interpolation is redundant when using
logger.exception.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/aap_eda/api/views/eda_credential.py` around lines 328 - 332, Remove explicit exception interpolation from logger.exception calls across the three sites: in src/aap_eda/api/views/eda_credential.py lines 328-332, update the handler around the credential plugin call and remove the unused err binding if applicable; in src/aap_eda/core/management/commands/dispatcherctl.py lines 141-142, retain error_msg for the raised CommandError but log only the command; in src/aap_eda/core/utils/external_sms.py line 50, log separate context without interpolating msg while preserving msg for the re-raised error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/aap_eda/tasks/orchestrator.py`:
- Line 261: Replace LOGGER.exception(msg) with traceback-free logging in the
HealthyQueueNotFoundError fallback handlers at the three affected locations,
using LOGGER.error(msg) or an appropriate warning level. Preserve exception
logging for unexpected failures and update only these deliberate
no-healthy-queue paths.
---
Nitpick comments:
In `@src/aap_eda/api/views/eda_credential.py`:
- Around line 328-332: Remove explicit exception interpolation from
logger.exception calls across the three sites: in
src/aap_eda/api/views/eda_credential.py lines 328-332, update the handler around
the credential plugin call and remove the unused err binding if applicable; in
src/aap_eda/core/management/commands/dispatcherctl.py lines 141-142, retain
error_msg for the raised CommandError but log only the command; in
src/aap_eda/core/utils/external_sms.py line 50, log separate context without
interpolating msg while preserving msg for the re-raised error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c3ce447-ded9-40b1-978a-6f9c3dbcf3c6
📒 Files selected for processing (56)
src/aap_eda/analytics/analytics_collectors.pysrc/aap_eda/analytics/collector.pysrc/aap_eda/analytics/utils.pysrc/aap_eda/api/serializers/activation.pysrc/aap_eda/api/views/activation.pysrc/aap_eda/api/views/credential_type.pysrc/aap_eda/api/views/eda_credential.pysrc/aap_eda/api/views/event_stream.pysrc/aap_eda/api/views/external_event_stream.pysrc/aap_eda/conf/registry.pysrc/aap_eda/core/health.pysrc/aap_eda/core/management/commands/dispatcherctl.pysrc/aap_eda/core/management/commands/dispatcherd.pysrc/aap_eda/core/models/project.pysrc/aap_eda/core/tasking/__init__.pysrc/aap_eda/core/utils/credential_plugins.pysrc/aap_eda/core/utils/credentials.pysrc/aap_eda/core/utils/external_sms.pysrc/aap_eda/core/utils/rulebook.pysrc/aap_eda/services/activation/activation_manager.pysrc/aap_eda/services/activation/drools_cleanup.pysrc/aap_eda/services/activation/engine/kubernetes.pysrc/aap_eda/services/activation/engine/podman.pysrc/aap_eda/services/activation/engine/ports.pysrc/aap_eda/services/pg_notify.pysrc/aap_eda/services/sync_certs.pysrc/aap_eda/tasks/orchestrator.pysrc/aap_eda/tasks/project.pysrc/aap_eda/utils/__init__.pysrc/aap_eda/wsapi/consumers.pytests/integration/analytics/test_analytics_collectors.pytests/integration/analytics/test_collector.pytests/integration/analytics/test_utils.pytests/integration/api/test_activation.pytests/integration/api/test_credential_type.pytests/integration/api/test_eda_credential.pytests/integration/api/test_event_stream.pytests/integration/api/test_event_stream_basic.pytests/integration/api/test_project.pytests/integration/core/test_dispatcherd_real.pytests/integration/core/test_health.pytests/integration/core/test_rulebook.pytests/integration/management/test_dispatcherd_commands.pytests/integration/services/activation/engine/test_kubernetes.pytests/integration/services/activation/engine/test_podman.pytests/integration/services/activation/test_drools_cleanup.pytests/integration/services/activation/test_manager.pytests/integration/services/test_sync_certs.pytests/integration/tasks/test_project_dispatcherd.pytests/integration/tasks/test_projects.pytests/integration/wsapi/test_consumer.pytests/unit/services/test_pg_notify.pytests/unit/test_credential_plugins.pytests/unit/test_credential_validation.pytests/unit/test_external_sms.pytests/unit/test_orchestrator.py
🚧 Files skipped from review as they are similar to previous changes (43)
- tests/integration/management/test_dispatcherd_commands.py
- src/aap_eda/services/activation/engine/ports.py
- tests/integration/tasks/test_project_dispatcherd.py
- src/aap_eda/api/views/activation.py
- tests/integration/api/test_credential_type.py
- tests/unit/services/test_pg_notify.py
- src/aap_eda/analytics/analytics_collectors.py
- src/aap_eda/core/management/commands/dispatcherd.py
- src/aap_eda/utils/init.py
- tests/unit/test_credential_plugins.py
- tests/unit/test_external_sms.py
- src/aap_eda/conf/registry.py
- src/aap_eda/core/utils/rulebook.py
- src/aap_eda/core/models/project.py
- tests/integration/api/test_eda_credential.py
- src/aap_eda/core/health.py
- tests/integration/api/test_project.py
- src/aap_eda/api/views/credential_type.py
- tests/integration/services/activation/test_drools_cleanup.py
- src/aap_eda/wsapi/consumers.py
- src/aap_eda/services/activation/engine/kubernetes.py
- tests/integration/tasks/test_projects.py
- src/aap_eda/core/utils/credential_plugins.py
- src/aap_eda/core/tasking/init.py
- tests/integration/services/activation/engine/test_kubernetes.py
- tests/integration/wsapi/test_consumer.py
- tests/unit/test_credential_validation.py
- src/aap_eda/core/utils/credentials.py
- tests/integration/core/test_rulebook.py
- tests/integration/services/activation/engine/test_podman.py
- tests/integration/api/test_activation.py
- tests/integration/analytics/test_analytics_collectors.py
- src/aap_eda/analytics/utils.py
- src/aap_eda/api/serializers/activation.py
- tests/integration/api/test_event_stream.py
- tests/integration/analytics/test_utils.py
- tests/integration/api/test_event_stream_basic.py
- tests/integration/core/test_dispatcherd_real.py
- src/aap_eda/services/activation/drools_cleanup.py
- tests/integration/services/activation/test_manager.py
- src/aap_eda/api/views/external_event_stream.py
- tests/integration/analytics/test_collector.py
- tests/unit/test_orchestrator.py
| "the administrator." | ||
| ) | ||
| LOGGER.error(msg) | ||
| LOGGER.exception(msg) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Do not emit tracebacks for the expected no-healthy-queue fallback.
HealthyQueueNotFoundError is intentionally raised when no healthy queue exists, so these handlers can run repeatedly during an outage. LOGGER.exception adds a full traceback to every event, causing avoidable log volume and potentially triggering noisy error alerts. Retain LOGGER.error(msg) without exc_info, or use an appropriate warning level if this condition is expected operationally.
The PR objective explicitly notes that logger.exception adds full traceback context; that behavior is useful for unexpected failures, but not for this deliberate fallback.
Suggested fix
- LOGGER.exception(msg)
+ LOGGER.error(msg)Apply this at Lines 261, 346, and 435.
Also applies to: 346-346, 435-435
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aap_eda/tasks/orchestrator.py` at line 261, Replace LOGGER.exception(msg)
with traceback-free logging in the HealthyQueueNotFoundError fallback handlers
at the three affected locations, using LOGGER.error(msg) or an appropriate
warning level. Preserve exception logging for unexpected failures and update
only these deliberate no-healthy-queue paths.
|
AlexSCorey
approved these changes
Jul 27, 2026
AlexSCorey
left a comment
AlexSCorey
left a comment
Member
There was a problem hiding this comment.
Looks good to me. Just some nitpicky things about the strings. I'll let you determine if you want to change them.
| f"YAML parsing error for credential {credential.id}: {e}" | ||
| except yaml.YAMLError: | ||
| logger.exception( | ||
| "YAML parsing error for credential" f" {credential.id}" |
Member
There was a problem hiding this comment.
This syntax/formatting feels weird to me.
Suggested change
| "YAML parsing error for credential" f" {credential.id}" | |
| f"YAML parsing error for credential {credential.id}" |
| except Exception: | ||
| logger.exception( | ||
| f"Unexpected error processing credential {credential.id}: {e}" | ||
| "Unexpected error processing credential" f" {credential.id}" |
ptoscano
requested changes
Jul 28, 2026
ptoscano
left a comment
ptoscano
left a comment
Contributor
There was a problem hiding this comment.
As I mentioned earlier, I don't think we ought to convert every logging.error() to logging.exception(). I think it would add too much information into the log file, and in many cases the benefit is negligible.
jcraiglo1
marked this pull request as draft
July 30, 2026 19:03
auto-merge was automatically disabled
July 30, 2026 19:03
Pull request was converted to draft
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://redhat.atlassian.net/browse/AAP-77394
One of multiple PRs addressing sonar quality gate issues. Sonar Cloud believes that we should use logger.exception in exception handlers instead of error. This PR addresses that and will remove ~96 issues from sonar.
Attached is a document describing the actions taken here.
sonarcloud-remediation-report.md
Summary by CodeRabbit
Bug Fixes
exception-level logging so tracebacks are captured.Tests
exception-level logging and the shortened log message formats.