sonar issues: Refactor functions with high complexity#1607
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThe PR refactors helper methods across API, core validation, activation and project services, task dispatch, and audit persistence, and adds queue-resolution tests. ChangesAPI, OpenAPI, and role setup
Core validation helpers
Activation, project, SCM, and task flow helpers
Audit persistence helpers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
src/aap_eda/core/management/commands/create_initial_data.py (1)
2757-2791: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid mutating the shared RBAC permissions list.
_create_org_admin_role()extends/appends to the caller-ownedpermissionslist. The current call order relies on_create_use_role()running before this helper; making a localorg_permissionscopy keeps role permissions isolated and avoids accidental privilege leakage if this helper is reused or reordered.Proposed fix
def _create_org_admin_role(self, cls, ct, permissions): """Create org-level admin role for a resource type.""" + org_permissions = list(permissions) name = self._format_role_name(cls, "Organization {verbose} Admin") org_ct = self.content_type_model.objects.get(model="organization") @@ - permissions.extend( + org_permissions.extend( DABPermission.objects.filter( content_type=ct, codename__startswith="add_" ) @@ - if org_view_permission and org_view_permission not in permissions: - permissions.append(org_view_permission) + if org_view_permission and org_view_permission not in org_permissions: + org_permissions.append(org_view_permission) - org_role.permissions.set(permissions) + org_role.permissions.set(org_permissions) if created: self.stdout.write( f"Added role {org_role.name} with " - f"{len(permissions)} permissions to itself" + f"{len(org_permissions)} permissions to itself" )🤖 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/core/management/commands/create_initial_data.py` around lines 2757 - 2791, The _create_org_admin_role helper is mutating the caller-owned permissions list, which can leak permissions across roles if call order changes or the helper is reused. Update _create_org_admin_role to work on a local copy (for example, derive a separate org-specific permissions collection before extending it with the add_* permissions and the organization view permission), then pass that isolated list to org_role.permissions.set so the original permissions list remains unchanged.Source: Path instructions
🤖 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/core/models/mixins.py`:
- Around line 68-83: The update_fields handling in the save mixin assumes
kwargs["update_fields"] is a mutable list, which can break when a non-list
iterable or None is passed. In the mixin method that checks status and
status_message, normalize update_fields before using "in" or append by
converting it to a mutable sequence (and handling None safely), then continue
the existing status validation and status_message synchronization logic.
In `@src/aap_eda/core/models/rulebook_process.py`:
- Around line 107-122: Normalize update_fields before using it in
RulebookProcess save handling: the current logic in the update_fields
validation/mutation path assumes a mutable list and can break for other
iterables or None. In the method that checks has_status and has_status_message,
convert kwargs["update_fields"] to a list up front, assign the normalized list
back into kwargs, and then perform the membership checks and append
"status_message" safely.
In `@src/aap_eda/core/utils/credentials.py`:
- Around line 225-240: The credential validation in the input handling logic is
treating boolean False like a missing value and then calling strip on it, which
breaks required boolean fields. Update the checks in the validation path around
the existing user_input/default handling so that default values are accepted
when they are not None rather than relying on truthiness, and only call .strip()
when user_input is a string. Make the fix in the same validation helper that
contains the user_input type check and the required/blank validation, and apply
the same adjustment to the related validation block mentioned in the comment.
- Around line 317-320: The optional gpg_public_key path in the credentials
validation flow is still calling _validate_gpg_public_key even when user_input
is absent, which turns an omitted optional field into an error. Update the
validation branch in the credentials utility so the gpg_public_key check is
skipped when user_input is None or otherwise empty, and only call
_validate_gpg_public_key when a value is actually provided. Keep the existing
errors[display_field] assignment behavior for real validation failures.
- Around line 371-374: The schema validation in the credentials flow currently
validates only the main fields, while metadata field definitions collected by
_get_id_fields(schema, "metadata") are skipped. Update the validation path
around _validate_field_ids and _validate_schema_fields in the schema-handling
logic to also run the same checks against metadata field definitions, using the
existing metadata collection and related helpers so invalid metadata schemas are
rejected early.
In `@src/aap_eda/core/validators.py`:
- Around line 121-134: The OCI reference parsing in _validate_oci_tag currently
marks any `@sha256` or `@sha512` value as digest=True and then skips validation,
allowing malformed digests to pass through. Update the digest-handling path in
_validate_oci_tag (and the related callers around the referenced lines) so that
digest references are explicitly validated for the expected hex format/length
instead of being returned unchecked, while preserving the existing tag parsing
behavior for non-digest references.
- Around line 137-140: The OCI name validation regex in _validate_oci_name
currently starts with a malformed nested character class that can allow invalid
names like ones beginning with “[”. Update the regex in validators.py by fixing
the leading token in _validate_oci_name so it uses the intended first-segment
pattern from the current expression, and keep the rest of the validation logic
unchanged.
In `@src/aap_eda/services/activation/engine/ports.py`:
- Around line 21-30: find_ports() and _extract_port() are letting invalid
non-dict context values raise TypeError instead of the expected
ActivationStartError. Add context type validation back in find_ports() before
iterating sources, and in _extract_port() catch template-rendering TypeError
from the rendering path and re-raise ActivationStartError with the same context.
Use the existing find_ports and _extract_port symbols to keep the contract
consistent for callers and tests.
In `@src/aap_eda/services/project/imports.py`:
- Around line 232-240: The rulebook scan in the import flow currently trusts
os.walk() and then passes each discovered path into _safe_load_rulebook(), which
can follow symlinks and allow reads outside the checkout. Update the traversal
in the rulebook import logic to reject symlinked files and any path that
resolves outside rulebooks_dir before calling _safe_load_rulebook(); use the
existing path handling in the imports.py loop to validate each candidate path
and keep the scan confined to the repository root.
In `@src/aap_eda/services/project/scm.py`:
- Around line 186-188: Missing gpg_public_key validation can let
_setup_gpg_credential or the gpg_key_file.write path raise TypeError before
clone() reaches its ScmError handling. Add an explicit check for the GPG public
key before any file write in the scm flow, and make sure
clone()/_setup_gpg_credential fail with a sanitized ScmError so cleanup and
finalization still run.
In `@src/aap_eda/utils/openapi.py`:
- Around line 20-25: The OpenAPI field mapping is using the wrong types for
audit filters, so update _FIELD_TYPE_MAP in openapi.py to map DateTimeField to
OpenApiTypes.DATETIME for created_at, fired_at, and received_at, and change
IntegerField from OpenApiTypes.NUMBER to OpenApiTypes.INT; keep the existing
CharField and BooleanField mappings unchanged.
In `@src/aap_eda/wsapi/consumers.py`:
- Around line 376-379: The matching-event loop in consumers.py can fail on
malformed entries because `event_meta.pop("meta")` raises `KeyError` when
`"meta"` is missing. Update the `matching_events` processing in the handler to
safely read `meta` with a default or a presence check before popping, and keep
the existing skip behavior so bad items are ignored instead of breaking action
ingestion.
---
Nitpick comments:
In `@src/aap_eda/core/management/commands/create_initial_data.py`:
- Around line 2757-2791: The _create_org_admin_role helper is mutating the
caller-owned permissions list, which can leak permissions across roles if call
order changes or the helper is reused. Update _create_org_admin_role to work on
a local copy (for example, derive a separate org-specific permissions collection
before extending it with the add_* permissions and the organization view
permission), then pass that isolated list to org_role.permissions.set so the
original permissions list remains unchanged.
🪄 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: Enterprise
Run ID: 6be38f3c-618f-4a93-9c37-fe45986aa650
📒 Files selected for processing (17)
src/aap_eda/api/metadata.pysrc/aap_eda/api/views/root.pysrc/aap_eda/core/management/commands/create_initial_data.pysrc/aap_eda/core/models/mixins.pysrc/aap_eda/core/models/rulebook_process.pysrc/aap_eda/core/utils/credentials.pysrc/aap_eda/core/validators.pysrc/aap_eda/services/activation/activation_manager.pysrc/aap_eda/services/activation/engine/kubernetes.pysrc/aap_eda/services/activation/engine/ports.pysrc/aap_eda/services/project/imports.pysrc/aap_eda/services/project/scm.pysrc/aap_eda/tasks/activation_request_queue.pysrc/aap_eda/tasks/orchestrator.pysrc/aap_eda/utils/openapi.pysrc/aap_eda/wsapi/consumers.pytests/unit/test_orchestrator.py
| if user_input is None: | ||
| if default: | ||
| inputs[field] = default | ||
| if required and not default: | ||
| errors[display_field] = ["Cannot be blank"] | ||
| return | ||
| elif not isinstance(user_input, (str, bool)): | ||
| msg = ( | ||
| "Input fields must have a boolean or string " | ||
| f"value. The value provided in the '{field}' " | ||
| f"field is of type " | ||
| f"{type(user_input).__name__}." | ||
| ) | ||
| errors[display_field] = [msg] | ||
| return | ||
| elif required and len(user_input.strip()) == 0: |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Handle boolean False without treating it as blank.
default=False is skipped by if default, and a required boolean input like False reaches user_input.strip(), causing an AttributeError instead of validation. Use default is not None and only call .strip() for strings.
Proposed fix
if user_input is None:
- if default:
+ if default is not None:
inputs[field] = default
- if required and not default:
+ if required and default is None:
errors[display_field] = ["Cannot be blank"]
return
@@
- elif required and len(user_input.strip()) == 0:
+ elif required and isinstance(user_input, str) and len(user_input.strip()) == 0:
errors[display_field] = ["Cannot be blank"]
returnAlso applies to: 255-258
🤖 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/core/utils/credentials.py` around lines 225 - 240, The credential
validation in the input handling logic is treating boolean False like a missing
value and then calling strip on it, which breaks required boolean fields. Update
the checks in the validation path around the existing user_input/default
handling so that default values are accepted when they are not None rather than
relying on truthiness, and only call .strip() when user_input is a string. Make
the fix in the same validation helper that contains the user_input type check
and the required/blank validation, and apply the same adjustment to the related
validation block mentioned in the comment.
There was a problem hiding this comment.
This is a valid bug that does need addressing however, this PR is not the place for it.
There was a problem hiding this comment.
@jcraiglo1 Understood — thanks for clarifying. I agree this looks like a real issue, but it doesn’t need to block #1607 if the goal here is just the Sonar-driven refactor.
If helpful, I can open a small follow-up issue to track the boolean-handling fix in src/aap_eda/core/utils/credentials.py.
(・x・)
| id_fields = _get_id_fields(schema) | ||
| metadata_fields = _get_id_fields(schema, "metadata") | ||
| _validate_field_ids(id_fields, errors) | ||
| formats_found = _validate_schema_fields(fields, errors) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate metadata field definitions too.
metadata IDs are collected for required-field checks, but metadata definitions are not passed through _validate_field_ids / _validate_schema_fields. Invalid metadata schemas can be accepted, then later crash validate_inputs(..., section="metadata") in serializers that validate credential input-source metadata.
Proposed fix
id_fields = _get_id_fields(schema)
metadata_fields = _get_id_fields(schema, "metadata")
_validate_field_ids(id_fields, errors)
- formats_found = _validate_schema_fields(fields, errors)
+ _validate_field_ids(metadata_fields, errors)
+ formats_found = _validate_schema_fields(fields, errors)
+ formats_found.extend(
+ _validate_schema_fields(schema.get("metadata", []), errors)
+ )📝 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.
| id_fields = _get_id_fields(schema) | |
| metadata_fields = _get_id_fields(schema, "metadata") | |
| _validate_field_ids(id_fields, errors) | |
| formats_found = _validate_schema_fields(fields, errors) | |
| id_fields = _get_id_fields(schema) | |
| metadata_fields = _get_id_fields(schema, "metadata") | |
| _validate_field_ids(id_fields, errors) | |
| _validate_field_ids(metadata_fields, errors) | |
| formats_found = _validate_schema_fields(fields, errors) | |
| formats_found.extend( | |
| _validate_schema_fields(schema.get("metadata", []), errors) | |
| ) |
🤖 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/core/utils/credentials.py` around lines 371 - 374, The schema
validation in the credentials flow currently validates only the main fields,
while metadata field definitions collected by _get_id_fields(schema, "metadata")
are skipped. Update the validation path around _validate_field_ids and
_validate_schema_fields in the schema-handling logic to also run the same checks
against metadata field definitions, using the existing metadata collection and
related helpers so invalid metadata schemas are rejected early.
There was a problem hiding this comment.
This essentially boils down to a feature request and out of the scope of this PR.
There was a problem hiding this comment.
@jcraiglo1 Understood — that makes sense for a complexity-focused refactor in #1607. I’m okay deferring this from the current PR.
If you want, I can open a follow-up issue to track validating metadata definitions in src/aap_eda/core/utils/credentials.py separately.
| for root, _dirs, files in os.walk(rulebooks_dir): | ||
| for filename in files: | ||
| path = os.path.join(root, filename) | ||
| _base, ext = os.path.splitext(filename) | ||
| if ext not in YAML_EXTENSIONS: | ||
| continue | ||
| try: | ||
| info = self._try_load_rulebook(rulebooks_dir, path) | ||
| except Exception: | ||
| logger.error( | ||
| "Unexpected exception when scanning file %s." | ||
| " Skipping.", | ||
| path, | ||
| exc_info=settings.DEBUG, | ||
| ) | ||
| continue | ||
| if not info: | ||
| logger.warning("Not a rulebook file: %s", path) | ||
| continue | ||
| yield info | ||
| path = os.path.join(root, filename) | ||
| info = self._safe_load_rulebook(rulebooks_dir, path) | ||
| if info: | ||
| yield info |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Block symlink/path-escape reads during rulebook scan.
os.walk() + open() will follow symlinked files from an untrusted repo. A crafted rulebook symlink can make the importer read files outside the checkout directory.
Suggested fix
for root, _dirs, files in os.walk(rulebooks_dir):
for filename in files:
_base, ext = os.path.splitext(filename)
if ext not in YAML_EXTENSIONS:
continue
path = os.path.join(root, filename)
+ real_path = os.path.realpath(path)
+ real_root = os.path.realpath(rulebooks_dir)
+ if not real_path.startswith(f"{real_root}{os.sep}") or os.path.islink(path):
+ logger.warning("Skipping non-local or symlinked rulebook file: %s", path)
+ continue
info = self._safe_load_rulebook(rulebooks_dir, path)
if info:
yield infoAs per path instructions, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 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.
| for root, _dirs, files in os.walk(rulebooks_dir): | |
| for filename in files: | |
| path = os.path.join(root, filename) | |
| _base, ext = os.path.splitext(filename) | |
| if ext not in YAML_EXTENSIONS: | |
| continue | |
| try: | |
| info = self._try_load_rulebook(rulebooks_dir, path) | |
| except Exception: | |
| logger.error( | |
| "Unexpected exception when scanning file %s." | |
| " Skipping.", | |
| path, | |
| exc_info=settings.DEBUG, | |
| ) | |
| continue | |
| if not info: | |
| logger.warning("Not a rulebook file: %s", path) | |
| continue | |
| yield info | |
| path = os.path.join(root, filename) | |
| info = self._safe_load_rulebook(rulebooks_dir, path) | |
| if info: | |
| yield info | |
| for root, _dirs, files in os.walk(rulebooks_dir): | |
| for filename in files: | |
| _base, ext = os.path.splitext(filename) | |
| if ext not in YAML_EXTENSIONS: | |
| continue | |
| path = os.path.join(root, filename) | |
| real_path = os.path.realpath(path) | |
| real_root = os.path.realpath(rulebooks_dir) | |
| if not real_path.startswith(f"{real_root}{os.sep}") or os.path.islink(path): | |
| logger.warning("Skipping non-local or symlinked rulebook file: %s", path) | |
| continue | |
| info = self._safe_load_rulebook(rulebooks_dir, path) | |
| if info: | |
| yield info |
🤖 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/project/imports.py` around lines 232 - 240, The rulebook
scan in the import flow currently trusts os.walk() and then passes each
discovered path into _safe_load_rulebook(), which can follow symlinks and allow
reads outside the checkout. Update the traversal in the rulebook import logic to
reject symlinked files and any path that resolves outside rulebooks_dir before
calling _safe_load_rulebook(); use the existing path handling in the imports.py
loop to validate each candidate path and keep the scan confined to the
repository root.
Source: Path instructions
There was a problem hiding this comment.
This is a pre-existing issue and may be worth looking into for hardening. But it is outside of the current scope of this PR.
There was a problem hiding this comment.
@jcraiglo1 Understood — thanks for clarifying. Agreed that this looks like a pre-existing hardening opportunity rather than something this refactor needs to take on.
If you'd like, I can open a small follow-up GitHub issue to track the symlink/path-escape check in src/aap_eda/services/project/imports.py separately.
| gpg_key_file, gpg_home_dir = cls._setup_gpg_credential( | ||
| gpg_credential, extra_vars, env_vars | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate missing GPG key before file write (currently can crash before error handling).
If gpg_public_key is absent, gpg_key_file.write(gpg_key) raises TypeError before clone() enters its try/except/finally, so cleanup/sanitized ScmError handling is bypassed.
Suggested fix
def _setup_gpg_credential(cls, gpg_credential, extra_vars, env_vars):
"""Set up GPG credential for commit verification."""
gpg_key_file = None
gpg_home_dir = None
if gpg_credential:
gpg_inputs = credentials.get_resolved_secrets(gpg_credential)
gpg_key = gpg_inputs.get("gpg_public_key")
+ if not gpg_key:
+ raise ScmError("Missing gpg_public_key in credential")
gpg_key_file = tempfile.NamedTemporaryFile("w+t")
gpg_key_file.write(gpg_key)As per path instructions, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 262-266
🤖 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/project/scm.py` around lines 186 - 188, Missing
gpg_public_key validation can let _setup_gpg_credential or the
gpg_key_file.write path raise TypeError before clone() reaches its ScmError
handling. Add an explicit check for the GPG public key before any file write in
the scm flow, and make sure clone()/_setup_gpg_credential fail with a sanitized
ScmError so cleanup and finalization still run.
Source: Path instructions
There was a problem hiding this comment.
This could be it's own ticket but again, pre-existing and outside of the scope. This PR should really be keeping the exact functionality and reducing computational complexity.
There was a problem hiding this comment.
@jcraiglo1 Understood — that makes sense for #1607.
Given the stated goal here is to preserve exact behavior and only reduce complexity, I’m fine leaving this out of scope for this PR. If you want, I can open a follow-up GitHub issue for the missing gpg_public_key validation in src/aap_eda/services/project/scm.py.
(｡•́︿•̀｡)🐇
| _FIELD_TYPE_MAP = { | ||
| models.CharField: OpenApiTypes.STR, | ||
| models.IntegerField: OpenApiTypes.NUMBER, | ||
| models.DateField: OpenApiTypes.DATETIME, | ||
| models.BooleanField: OpenApiTypes.BOOL, | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and related usage.
git ls-files src/aap_eda/utils/openapi.py
wc -l src/aap_eda/utils/openapi.py
cat -n src/aap_eda/utils/openapi.py
printf '\n--- references ---\n'
rg -n "OpenApiTypes|_FIELD_TYPE_MAP|generate_query_params|DateField|IntegerField" src/aap_eda -SRepository: ansible/eda-server
Length of output: 9780
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/aap_eda/utils/openapi.pyRepository: ansible/eda-server
Length of output: 2183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/aap_eda/utils/openapi.py
rg -n "OpenApiTypes|_FIELD_TYPE_MAP|generate_query_params|DateField|IntegerField" src/aap_eda -SRepository: ansible/eda-server
Length of output: 9258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/aap_eda/utils/openapi.py
rg -n "OpenApiTypes|_FIELD_TYPE_MAP|generate_query_params|DateField|IntegerField" src/aap_eda -SRepository: ansible/eda-server
Length of output: 9258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/aap_eda/utils/openapi.py
printf '\n---\n'
rg -n "OpenApiTypes|_FIELD_TYPE_MAP|generate_query_params|DateField|IntegerField" src/aap_eda -SRepository: ansible/eda-server
Length of output: 9263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/aap_eda/api/serializers/rulebook.py --view expanded || true
printf '\n---\n'
ast-grep outline src/aap_eda/core/models/activation.py --view expanded || true
printf '\n---\n'
ast-grep outline src/aap_eda/core/models/event_stream.py --view expanded || true
printf '\n---\n'
rg -n "class AuditRuleSerializer|class AuditEventSerializer|Meta\.model|DateField|IntegerField|BigIntegerField|BooleanField" src/aap_eda/api/serializers/rulebook.py src/aap_eda/api/serializers/activation.py src/aap_eda/core/models/activation.py src/aap_eda/core/models/event_stream.py -SRepository: ansible/eda-server
Length of output: 8898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '63,260p' src/aap_eda/api/serializers/rulebook.py
printf '\n--- model definitions referenced by these serializers ---\n'
rg -n "class .*Audit|class .*Rule|class .*Event|class Meta:" src/aap_eda/api/serializers/rulebook.py src/aap_eda/core/models -SRepository: ansible/eda-server
Length of output: 8408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '63,260p' src/aap_eda/api/serializers/rulebook.py
printf '\n---\n'
rg -n "class .*Audit|class .*Rule|class .*Event|Meta:" src/aap_eda/api/serializers/rulebook.py src/aap_eda/core/models -SRepository: ansible/eda-server
Length of output: 8354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' src/aap_eda/core/models/rulebook.pyRepository: ansible/eda-server
Length of output: 4392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' src/aap_eda/core/models/rulebook.pyRepository: ansible/eda-server
Length of output: 4392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' src/aap_eda/core/models/rulebook.py
printf '\n---\n'
rg -n "models\.(DateField|DateTimeField|IntegerField|BigIntegerField|ForeignKey|OneToOneField|ManyToManyField)" src/aap_eda/core/models/rulebook.py -n -SRepository: ansible/eda-server
Length of output: 5120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,180p' src/aap_eda/core/models/rulebook.pyRepository: ansible/eda-server
Length of output: 4392
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/aap_eda/core/models/base.py
printf '\n---\n'
sed -n '1,260p' src/aap_eda/core/models/rulebook_process.py
printf '\n---\n'
sed -n '1,180p' src/aap_eda/core/models/job.py
printf '\n---\n'
sed -n '1,160p' src/aap_eda/core/models/organization.pyRepository: ansible/eda-server
Length of output: 13478
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/aap_eda/core/models/base.py
printf '\n---\n'
sed -n '1,260p' src/aap_eda/core/models/rulebook_process.py
printf '\n---\n'
sed -n '1,180p' src/aap_eda/core/models/job.py
printf '\n---\n'
sed -n '1,160p' src/aap_eda/core/models/organization.pyRepository: ansible/eda-server
Length of output: 13478
Map the audit filter types to the actual field classes. created_at, fired_at, and received_at are DateTimeFields but currently fall back to STR; add a DateTimeField -> OpenApiTypes.DATETIME mapping. IntegerField should use OpenApiTypes.INT instead of NUMBER.
🤖 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/utils/openapi.py` around lines 20 - 25, The OpenAPI field mapping
is using the wrong types for audit filters, so update _FIELD_TYPE_MAP in
openapi.py to map DateTimeField to OpenApiTypes.DATETIME for created_at,
fired_at, and received_at, and change IntegerField from OpenApiTypes.NUMBER to
OpenApiTypes.INT; keep the existing CharField and BooleanField mappings
unchanged.
There was a problem hiding this comment.
Updating this would cause the API spec and is very much out of the scope of this PR.
There was a problem hiding this comment.
@jcraiglo1 Fair point — agreed that this would change the generated API spec, so it doesn’t fit a complexity-only refactor PR like #1607.
I’m good deferring this. If you want, I can open a follow-up GitHub issue to track the OpenAPI type mapping update separately.
🐇💭
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #1607 +/- ##
==========================================
+ Coverage 92.48% 92.75% +0.26%
==========================================
Files 244 244
Lines 11248 11349 +101
==========================================
+ Hits 10403 10527 +124
+ Misses 845 822 -23
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: Jacob Craiglow <jacobcraiglow1@gmail.com>
Signed-off-by: Jacob Craiglow <jacobcraiglow1@gmail.com>
Signed-off-by: Jacob Craiglow <jacobcraiglow1@gmail.com>
Signed-off-by: Jacob Craiglow <jacobcraiglow1@gmail.com>
jcraiglo1
force-pushed
the
sonar/S3776-complexity
branch
from
8ced72b to
4f28915
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/aap_eda/services/activation/activation_manager.py (1)
899-901: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for the
ContainerEngineErrorbranch.This helper now owns the
WORKERS_OFFLINEtransition and the early return frommonitor(). Since this PR is meant to be behavior-preserving and Codecov still shows uncovered lines in this file, it’s worth pinning this path with a test so worker outages don’t regress into the missing-container flow.Also applies to: 984-1015
🤖 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 899 - 901, Add a unit test that exercises the ContainerEngineError path in ActivationManager.monitor() by stubbing _get_container_status() to return an engine_error and asserting the method exits early without taking the missing-container branch. Make the test pin the WORKERS_OFFLINE transition owned by this helper, and verify any expected status/update behavior around the container_status handling so the early return remains behavior-preserving. Use ActivationManager, monitor(), and _get_container_status() as the key symbols when locating the branch.
🤖 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/core/validators.py`:
- Around line 121-134: The `_parse_image_url()` logic in `validators.py` is
handling `name:tag@digest` incorrectly by splitting at the digest marker too
early and leaving the tag attached to `name`. Update `_parse_image_url()` to
strip and record the digest first, then parse the remaining `name[:tag]` portion
so valid references like `repo:latest@sha256:...` produce `name="repo"` and
`tag="latest"` before `_validate_oci_name()` runs.
In `@src/aap_eda/services/activation/engine/kubernetes.py`:
- Around line 586-632: The _watch_pod_deletion flow in Kubernetes cleanup can
falsely report success when watcher.stream() times out or emits no DELETED
event. Update _watch_pod_deletion so it only logs “Job ... is cleaned up.” and
returns after an actual DELETED event or a follow-up check with
self.client.core_api.list_namespaced_pod confirms the pod no longer exists. Use
the existing watch.Watch loop, the DELETED event handling, and the
ContainerCleanupError path to keep retries and failures consistent.
- Around line 291-312: The _call_k8s_api helper is making non-idempotent
create_namespaced_job/create_namespaced_service/create_namespaced_secret calls
retryable in a way that can turn a transient server-side success into a fatal
409 Conflict and trigger cleanup; update the create path to be idempotent by
using create-or-replace semantics or by handling 409 explicitly and adopting the
existing resource before returning. Also tighten _watch_pod_deletion so it only
reports success after verifying the target resource is actually gone, rather
than returning True on timeout or 404, to avoid masking leaks during cleanup.
---
Nitpick comments:
In `@src/aap_eda/services/activation/activation_manager.py`:
- Around line 899-901: Add a unit test that exercises the ContainerEngineError
path in ActivationManager.monitor() by stubbing _get_container_status() to
return an engine_error and asserting the method exits early without taking the
missing-container branch. Make the test pin the WORKERS_OFFLINE transition owned
by this helper, and verify any expected status/update behavior around the
container_status handling so the early return remains behavior-preserving. Use
ActivationManager, monitor(), and _get_container_status() as the key symbols
when locating the branch.
🪄 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: Enterprise
Run ID: 4574d4eb-36db-4cf7-9fbb-6874181cf994
📒 Files selected for processing (17)
src/aap_eda/api/metadata.pysrc/aap_eda/api/views/root.pysrc/aap_eda/core/management/commands/create_initial_data.pysrc/aap_eda/core/models/mixins.pysrc/aap_eda/core/models/rulebook_process.pysrc/aap_eda/core/utils/credentials.pysrc/aap_eda/core/validators.pysrc/aap_eda/services/activation/activation_manager.pysrc/aap_eda/services/activation/engine/kubernetes.pysrc/aap_eda/services/activation/engine/ports.pysrc/aap_eda/services/project/imports.pysrc/aap_eda/services/project/scm.pysrc/aap_eda/tasks/activation_request_queue.pysrc/aap_eda/tasks/orchestrator.pysrc/aap_eda/utils/openapi.pysrc/aap_eda/wsapi/consumers.pytests/unit/test_orchestrator.py
✅ Files skipped from review due to trivial changes (1)
- src/aap_eda/api/metadata.py
🚧 Files skipped from review as they are similar to previous changes (12)
- src/aap_eda/core/models/mixins.py
- src/aap_eda/api/views/root.py
- src/aap_eda/services/project/scm.py
- src/aap_eda/tasks/activation_request_queue.py
- src/aap_eda/utils/openapi.py
- src/aap_eda/core/models/rulebook_process.py
- tests/unit/test_orchestrator.py
- src/aap_eda/services/project/imports.py
- src/aap_eda/wsapi/consumers.py
- src/aap_eda/tasks/orchestrator.py
- src/aap_eda/core/utils/credentials.py
- src/aap_eda/core/management/commands/create_initial_data.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/aap_eda/services/activation/activation_manager.py (1)
899-901: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for the
ContainerEngineErrorbranch.This helper now owns the
WORKERS_OFFLINEtransition and the early return frommonitor(). Since this PR is meant to be behavior-preserving and Codecov still shows uncovered lines in this file, it’s worth pinning this path with a test so worker outages don’t regress into the missing-container flow.Also applies to: 984-1015
🤖 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 899 - 901, Add a unit test that exercises the ContainerEngineError path in ActivationManager.monitor() by stubbing _get_container_status() to return an engine_error and asserting the method exits early without taking the missing-container branch. Make the test pin the WORKERS_OFFLINE transition owned by this helper, and verify any expected status/update behavior around the container_status handling so the early return remains behavior-preserving. Use ActivationManager, monitor(), and _get_container_status() as the key symbols when locating the branch.
🤖 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/core/validators.py`:
- Around line 121-134: The `_parse_image_url()` logic in `validators.py` is
handling `name:tag@digest` incorrectly by splitting at the digest marker too
early and leaving the tag attached to `name`. Update `_parse_image_url()` to
strip and record the digest first, then parse the remaining `name[:tag]` portion
so valid references like `repo:latest@sha256:...` produce `name="repo"` and
`tag="latest"` before `_validate_oci_name()` runs.
In `@src/aap_eda/services/activation/engine/kubernetes.py`:
- Around line 586-632: The _watch_pod_deletion flow in Kubernetes cleanup can
falsely report success when watcher.stream() times out or emits no DELETED
event. Update _watch_pod_deletion so it only logs “Job ... is cleaned up.” and
returns after an actual DELETED event or a follow-up check with
self.client.core_api.list_namespaced_pod confirms the pod no longer exists. Use
the existing watch.Watch loop, the DELETED event handling, and the
ContainerCleanupError path to keep retries and failures consistent.
- Around line 291-312: The _call_k8s_api helper is making non-idempotent
create_namespaced_job/create_namespaced_service/create_namespaced_secret calls
retryable in a way that can turn a transient server-side success into a fatal
409 Conflict and trigger cleanup; update the create path to be idempotent by
using create-or-replace semantics or by handling 409 explicitly and adopting the
existing resource before returning. Also tighten _watch_pod_deletion so it only
reports success after verifying the target resource is actually gone, rather
than returning True on timeout or 404, to avoid masking leaks during cleanup.
---
Nitpick comments:
In `@src/aap_eda/services/activation/activation_manager.py`:
- Around line 899-901: Add a unit test that exercises the ContainerEngineError
path in ActivationManager.monitor() by stubbing _get_container_status() to
return an engine_error and asserting the method exits early without taking the
missing-container branch. Make the test pin the WORKERS_OFFLINE transition owned
by this helper, and verify any expected status/update behavior around the
container_status handling so the early return remains behavior-preserving. Use
ActivationManager, monitor(), and _get_container_status() as the key symbols
when locating the branch.
🪄 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: Enterprise
Run ID: 4574d4eb-36db-4cf7-9fbb-6874181cf994
📒 Files selected for processing (17)
src/aap_eda/api/metadata.pysrc/aap_eda/api/views/root.pysrc/aap_eda/core/management/commands/create_initial_data.pysrc/aap_eda/core/models/mixins.pysrc/aap_eda/core/models/rulebook_process.pysrc/aap_eda/core/utils/credentials.pysrc/aap_eda/core/validators.pysrc/aap_eda/services/activation/activation_manager.pysrc/aap_eda/services/activation/engine/kubernetes.pysrc/aap_eda/services/activation/engine/ports.pysrc/aap_eda/services/project/imports.pysrc/aap_eda/services/project/scm.pysrc/aap_eda/tasks/activation_request_queue.pysrc/aap_eda/tasks/orchestrator.pysrc/aap_eda/utils/openapi.pysrc/aap_eda/wsapi/consumers.pytests/unit/test_orchestrator.py
✅ Files skipped from review due to trivial changes (1)
- src/aap_eda/api/metadata.py
🚧 Files skipped from review as they are similar to previous changes (12)
- src/aap_eda/core/models/mixins.py
- src/aap_eda/api/views/root.py
- src/aap_eda/services/project/scm.py
- src/aap_eda/tasks/activation_request_queue.py
- src/aap_eda/utils/openapi.py
- src/aap_eda/core/models/rulebook_process.py
- tests/unit/test_orchestrator.py
- src/aap_eda/services/project/imports.py
- src/aap_eda/wsapi/consumers.py
- src/aap_eda/tasks/orchestrator.py
- src/aap_eda/core/utils/credentials.py
- src/aap_eda/core/management/commands/create_initial_data.py
🛑 Comments failed to post (3)
src/aap_eda/core/validators.py (1)
121-134: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
name:tag@digestreferences are parsed incorrectly.When
pathcontains@sha256/@sha512,_parse_image_url()splits on@first and returns the entire left side asname. A valid reference likerepo:latest@sha256:...therefore becomesname == "repo:latest"and is rejected by_validate_oci_name(). Split off the digest first, then parse the remainingname[:tag]portion.🤖 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/core/validators.py` around lines 121 - 134, The `_parse_image_url()` logic in `validators.py` is handling `name:tag@digest` incorrectly by splitting at the digest marker too early and leaving the tag attached to `name`. Update `_parse_image_url()` to strip and record the digest first, then parse the remaining `name[:tag]` portion so valid references like `repo:latest@sha256:...` produce `name="repo"` and `tag="latest"` before `_validate_oci_name()` runs.src/aap_eda/services/activation/engine/kubernetes.py (2)
291-312: 🩺 Stability & Availability | 🟠 Major
Non-idempotent
create_*calls can trigger false cleanup on transient errors.The
_call_k8s_apihelper now wrapscreate_namespaced_job,create_namespaced_service, andcreate_namespaced_secret. If acreateoperation succeeds on the Kubernetes API server but the client receives a transient 5xx status and retries, the retry will likely fail with409 Conflict. Since409is not inK8S_API_TRANSIENT_STATUS_CODES, this triggers a fatalContainerEngineError. This causes thestart()method to executecleanup(), deleting resources that were actually created successfully.Recommendations:
- Implement Idempotency: Ensure
create_*methods use "create or replace" semantics or catch409to verify/adopt the existing resource instead of failing immediately.- Verify Cleanup Success: The
_watch_pod_deletionmethod returnsTrueeven if the watch loop times out or encounters404(Lines 605-609). Verify the resource is actually deleted before marking cleanup as successful to prevent masking leaks.Also applies to: 473-479, 541-547, 752-758
🤖 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` around lines 291 - 312, The _call_k8s_api helper is making non-idempotent create_namespaced_job/create_namespaced_service/create_namespaced_secret calls retryable in a way that can turn a transient server-side success into a fatal 409 Conflict and trigger cleanup; update the create path to be idempotent by using create-or-replace semantics or by handling 409 explicitly and adopting the existing resource before returning. Also tighten _watch_pod_deletion so it only reports success after verifying the target resource is actually gone, rather than returning True on timeout or 404, to avoid masking leaks during cleanup.
586-632: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cleanup can report success before the pod is actually gone.
If the watch times out or yields no
DELETEDevent, this code still logs"Job ... is cleaned up."and returns. That lets stop/cleanup finish while the pod may still exist, leaving orphaned resources behind. Only return success after a delete event or a follow-up list confirms the pod is absent.🧰 Tools
🪛 Ruff (0.15.18)
[warning] 625-625: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 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` around lines 586 - 632, The _watch_pod_deletion flow in Kubernetes cleanup can falsely report success when watcher.stream() times out or emits no DELETED event. Update _watch_pod_deletion so it only logs “Job ... is cleaned up.” and returns after an actual DELETED event or a follow-up check with self.client.core_api.list_namespaced_pod confirms the pod no longer exists. Use the existing watch.Watch loop, the DELETED event handling, and the ContainerCleanupError path to keep retries and failures consistent.
|
|
/run-e2e |
|
/run-atf-tests |
✅ Test Results - PASSEDSummary
Pass Rate: 75.4% |
4 participants
https://redhat.atlassian.net/browse/AAP-77394
One of multiple PRs addressing sonar quality gate issues. This PR is a little more complex. Sonarcloud now gates on function complexity. Sonarcloud has defined a set of rules to determine complexity. Here's a basic breakdown. The actual rules may vary slightly:
STRUCTURAL KEYWORDS | Base | Nesting Bonus
─────────────────────────────────────────────────────
if +1 +1 per level
for +1 +1 per level
while +1 +1 per level
except +1 +1 per level
CONTINUATION KEYWORDS | Base | Nesting Bonus
─────────────────────────────────────────────────────
elif +1 none
else +1 none
LOGICAL OPERATORS | Base | Nesting Bonus
─────────────────────────────────────────────────────
and +1 none
or +1 none
OTHER INCREMENTS | Base | Nesting Bonus
─────────────────────────────────────────────────────
ternary (x if cond else y) +1 none
recursion (self-call) +1 none
NESTING SOURCES (increase depth, but don't add points)
─────────────────────────────────────────────────────
if / for / while / try / with
nested functions / nested classes / lambdas
NO COST (zero points)
─────────────────────────────────────────────────────
try / finally / with / break / continue
return / raise / pass / import
Source: Interpreted from G. Ann Campbell's
"Cognitive Complexity" specification (SonarSource)
Sonarcloud gates at a complexity level of 15. All of these functions returned a complexity of over 15. All of these decompositions have the express purpose of reducing that complexity. Typically by the use of module level helper functions. No functionality was changed intentionally and tests pass without re-writing them.
NOTE: Exceptions can be made on a case by case basis as far as sonarcloud is concerned. I am not sure the orgs stance on this. If anyone feels that an exception should be made here please speak up. For example there is a function in a later PR that was copied from awx that I believe should have an exemption because it is not directly our code per se.
Attached is a document describing the actions taken here.
sonarcloud-remediation-report.md
Assisted-by: Claude
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests