chore: delete all fuctional of Cyndi#2322
Open
RostyslavKachan wants to merge 1 commit into
Open
Conversation
RHINENG-23296
Reviewer's GuideRemoves all legacy Cyndi/inventory.hosts integration and feature-flagged paths in favor of the new SystemPlatform + SystemGroupSet/OperatingSystem model, simplifying joins, filters, metrics, admin endpoints, tests, and local DB setup while standardizing how OS, groups, tags, and ownership are handled. Updated class diagram for join helpers and data models (class)classDiagram
class SystemPlatform {
UUID inventory_id
int rh_account_id
int operating_system_id
UUID tag_set_id
datetime last_evaluation
datetime last_upload
datetime updated_timestamp
datetime when_deleted
bool opt_out
bool advisor_evaluated
bool stale
jsonb sap_ids
bool ansible
bool mssql
int cve_count_cache
string host_type
string owner_id
}
class SystemGroupSet {
int id
jsonb groups
}
class SystemTagSet {
int id
jsonb tags
}
class OperatingSystem {
int id
string name
int major
int minor
OperatingSystem display_os_format()
list~SQL~ sort_columns()
}
class ManagerBase {
Query SystemGroupSet_join(Query query)
Query filter_allowed_groups(Query query, jsonb groups_column)
Query filter_kessel_workspace_opt_out(Query query, jsonb groups_column)
}
class FiltersModule {
Query _filter_system_by_tags(Query query, dict args, dict kwargs)
Query _filter_system_by_sap_sids(Query query, dict args, dict kwargs)
Query _filter_system_by_rhel_version(Query query, dict args, dict kwargs)
Query _filter_system_by_ansible(Query query, dict args, dict kwargs)
Query _filter_system_by_mssql(Query query, dict args, dict kwargs)
Query _filter_inventory_group_names(Query query, dict args, dict kwargs)
Query _filter_inventory_group_ids(Query query, dict args, dict kwargs)
}
class Handlers {
Query cve_handler_full_query()
Query cve_handler_unpatched_query()
Query system_handler_full_query()
Query system_handler_unpatched_query()
Query system_view_query()
Query report_handler_query()
Query dashboard_handler_query()
Query dashbar_handler_query()
Query vulnerabilities_handler_query()
Query status_handler_query()
}
ManagerBase --> SystemGroupSet : join
ManagerBase --> SystemPlatform : join
SystemPlatform --> SystemGroupSet : has tag_set_id
SystemPlatform --> SystemTagSet : has tag_set_id
SystemPlatform --> OperatingSystem : has operating_system_id
FiltersModule --> SystemGroupSet : uses groups
FiltersModule --> SystemPlatform : uses sap_ids
FiltersModule --> OperatingSystem : uses name, major, minor
Handlers --> ManagerBase : calls SystemGroupSet_join
Handlers --> FiltersModule : apply filters
Handlers --> OperatingSystem : uses display_os_format, sort_columns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
SC Environment Impact AssessmentOverall Impact: ⚪ NONE No SC Environment-specific impacts detected in this PR. What was checkedThis PR was automatically scanned for:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
_filter_system_by_rhel_version, the new loop buildsexprincorrectly (expr = expr if expr is None else expr | expr), which effectively discards prior conditions and never incorporates the current one; this should OR the existing expression with the current condition (e.g.,expr = cond if expr is None else expr | cond). - The new helper
SystemGroupSet_joinis a function but is named in PascalCase, which is inconsistent with the rest of the module’s snake_case functions; consider renaming it (e.g.,system_group_set_join) to match the project’s naming conventions and avoid confusion with classes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_filter_system_by_rhel_version`, the new loop builds `expr` incorrectly (`expr = expr if expr is None else expr | expr`), which effectively discards prior conditions and never incorporates the current one; this should OR the existing expression with the current condition (e.g., `expr = cond if expr is None else expr | cond`).
- The new helper `SystemGroupSet_join` is a function but is named in PascalCase, which is inconsistent with the rest of the module’s snake_case functions; consider renaming it (e.g., `system_group_set_join`) to match the project’s naming conventions and avoid confusion with classes.
## Individual Comments
### Comment 1
<location path="manager/filters.py" line_range="463" />
<code_context>
+ expr = OperatingSystem.major == int(version)
+ except (ValueError, TypeError):
+ continue
+ expr = expr if expr is None else expr | expr
+ if expr is not None:
+ query = query.where((OperatingSystem.name == "RHEL") & (expr))
</code_context>
<issue_to_address>
**issue (bug_risk):** The RHEL version filter is now logically incorrect; the OR expression uses `expr | expr` instead of accumulating conditions.
In `_filter_system_by_rhel_version`, the refactor broke the accumulation of per-version conditions. `expr = expr if expr is None else expr | expr` never incorporates the new condition and just ORs `expr` with itself, so `expr` either stays `None` (no filter) or is a no-op across iterations.
You likely want to accumulate `cond` instead, e.g.:
```python
for version in args["rhel_version"]:
try:
if "." in version:
major, minor = version.split(".", 1)
major, minor = int(major), int(minor)
cond = (OperatingSystem.major == major) & (OperatingSystem.minor == minor)
else:
cond = OperatingSystem.major == int(version)
except (ValueError, TypeError):
continue
expr = cond if expr is None else expr | cond
if expr is not None:
query = query.where((OperatingSystem.name == "RHEL") & expr)
```
As written, multi-version filters (and possibly single-version filters, depending on how Peewee treats `None`) will not behave as intended.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| expr = OperatingSystem.major == int(version) | ||
| except (ValueError, TypeError): | ||
| continue | ||
| expr = expr if expr is None else expr | expr |
There was a problem hiding this comment.
issue (bug_risk): The RHEL version filter is now logically incorrect; the OR expression uses expr | expr instead of accumulating conditions.
In _filter_system_by_rhel_version, the refactor broke the accumulation of per-version conditions. expr = expr if expr is None else expr | expr never incorporates the new condition and just ORs expr with itself, so expr either stays None (no filter) or is a no-op across iterations.
You likely want to accumulate cond instead, e.g.:
for version in args["rhel_version"]:
try:
if "." in version:
major, minor = version.split(".", 1)
major, minor = int(major), int(minor)
cond = (OperatingSystem.major == major) & (OperatingSystem.minor == minor)
else:
cond = OperatingSystem.major == int(version)
except (ValueError, TypeError):
continue
expr = cond if expr is None else expr | cond
if expr is not None:
query = query.where((OperatingSystem.name == "RHEL") & expr)As written, multi-version filters (and possibly single-version filters, depending on how Peewee treats None) will not behave as intended.
Collaborator
Author
|
good luck with review) |
Collaborator
Author
|
/retest |
1 similar comment
Collaborator
Author
|
/retest |
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
1 participant
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.
RHINENG-23296
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Remove legacy Cyndi inventory integration and rely solely on system_platform and related tables for system metadata, grouping, and filtering.
Enhancements:
Chores: