chore: cyndi replacement os display sort and dev data#2240
Open
pindo696 wants to merge 3 commits intoRedHatInsights:masterfrom
Open
chore: cyndi replacement os display sort and dev data#2240pindo696 wants to merge 3 commits intoRedHatInsights:masterfrom
pindo696 wants to merge 3 commits intoRedHatInsights:masterfrom
Conversation
Cyndi migration inventory hosts system profile os to operating system RHINENG-23544
Contributor
SC Environment Impact AssessmentOverall Impact: 🟢 LOW View full reportSummary
Detailed Findings🟢 LOW ImpactFeature flag change detected
Feature flag change detected
Feature flag change detected
Required Actions
This assessment was automatically generated. Please review carefully and consult with the ROSA Core team for critical/high impact changes. |
Reviewer's GuideMigrates dev data and query behavior from inventory system_profile OS fields to the normalized operating_system table behind the CYNDI_REPLICATION_REPLACEMENT feature flag, adding helper methods for OS display/sort and wiring the new join into CVE and system listing queries. ER diagram for system_platform and operating_system relationshiperDiagram
operating_system {
int id PK
string name
int major
int minor
string lifecycle_phase
bool usable_for_report
int cves_critical
int cves_important
int cves_moderate
int cves_low
int cves_unpatched_critical
int cves_unpatched_important
int cves_unpatched_moderate
int cves_unpatched_low
}
system_platform {
int id PK
uuid inventory_id
string display_name
int rh_account_id FK
string s3_url
json vmaas_json
string json_checksum
timestamp last_upload
timestamp last_evaluation
timestamp first_reported
timestamp last_updated
timestamp unchanged_since
timestamp advisor_evaluated
string advisor_checksum
timestamp advisor_unchanged_since
timestamp when_deleted
timestamp culled_timestamp
bool stale
bool opt_out
string host_type
int operating_system_id FK
}
operating_system ||--o{ system_platform : operating_system_id
Class diagram for OperatingSystem model and sorting/display helpersclassDiagram
class BaseModel
class OperatingSystem {
+int id
+string name
+int major
+int minor
+string lifecycle_phase
+bool usable_for_report
+int cves_critical
+int cves_important
+int cves_moderate
+int cves_low
+int cves_unpatched_critical
+int cves_unpatched_important
+int cves_unpatched_moderate
+int cves_unpatched_low
+display_os_format() SQLExpression
+sort_columns() List~Field~
}
class SystemPlatform {
+int id
+int operating_system_id
}
BaseModel <|-- OperatingSystem
OperatingSystem "1" <.. "*" SystemPlatform : referenced_by
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 3 issues, and left some high level feedback:
- In
_filter_system_by_rhel_version,OperatingSystem.major/minorare likely integer columns but you’re comparing them to raw string values fromversion.split, so it would be safer to cast those to integers (or explicitly cast in SQL) to avoid subtle type-mismatch issues. - The
expraccumulator in_filter_system_by_rhel_versionis initialized asNoneand then immediately OR-ed (expr | ...), which can break on the first iteration; consider initializing it to a false-y Peewee expression or building a list of conditions and combining them withreduce/operator.or_.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_filter_system_by_rhel_version`, `OperatingSystem.major/minor` are likely integer columns but you’re comparing them to raw string values from `version.split`, so it would be safer to cast those to integers (or explicitly cast in SQL) to avoid subtle type-mismatch issues.
- The `expr` accumulator in `_filter_system_by_rhel_version` is initialized as `None` and then immediately OR-ed (`expr | ...`), which can break on the first iteration; consider initializing it to a false-y Peewee expression or building a list of conditions and combining them with `reduce`/`operator.or_`.
## Individual Comments
### Comment 1
<location path="manager/cve_handler.py" line_range="206" />
<code_context>
"updated": SQL("updated"),
"first_reported": SQL("first_reported"),
- "os": OS_INFO_SORT,
+ "os": ([SQL("os")] if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT) else OS_INFO_SORT),
"advisory_available": SQL("advisory_available"),
"remediation": SQL("remediation_type_id"),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use `OperatingSystem.sort_columns()` when Unleash is enabled instead of `[SQL("os")]` to keep stable sort semantics.
This change switches the `os` sort key from the structured `OS_INFO_SORT` to the derived string column `SQL("os")` when Unleash is enabled, which can change ordering semantics (e.g., string vs numeric comparison, NULL handling) and tie sorting to the display format.
Since you added `OperatingSystem.sort_columns()`, you can use it here to keep the sort based on the underlying OS structure:
```python
"os": (
OperatingSystem.sort_columns()
if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT)
else OS_INFO_SORT
),
```
```suggestion
"os": (
OperatingSystem.sort_columns()
if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT)
else OS_INFO_SORT
),
```
</issue_to_address>
### Comment 2
<location path="manager/system_handler.py" line_range="502-506" />
<code_context>
"updated": SQL("updated"),
"first_reported": SQL("first_reported"),
- "os": OS_INFO_SORT,
+ "os": ([SQL("os")] if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT) else OS_INFO_SORT),
"advisory_available": SQL("advisory_available"),
"remediation": SQL("remediation_type_id"),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align system list OS sorting with `OperatingSystem.sort_columns()` instead of `SQL("os")`.
Using `[SQL("os")]` when Unleash is enabled changes sorting to use the rendered OS string, which can diverge from the previous structured sort. Since `OperatingSystem.sort_columns()` now exists, you could use:
```python
"os": (
OperatingSystem.sort_columns()
if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT)
else OS_INFO_SORT
),
```
to keep ordering consistent across both paths and avoid coupling sort behavior to the display expression.
```suggestion
"display_name": SystemPlatform.display_name,
"rules_evaluation": SystemPlatform.advisor_evaluated,
"updated": InventoryHosts.updated,
"os": (
OperatingSystem.sort_columns()
if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT)
else OS_INFO_SORT
),
}
```
</issue_to_address>
### Comment 3
<location path="manager/filters.py" line_range="458-467" />
<code_context>
if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT):
query = (query.switch(SystemPlatform).join(SystemGroupSet, JOIN.LEFT_OUTER))
+ query = (query.switch(SystemPlatform).join(OperatingSystem, JOIN.LEFT_OUTER,
</code_context>
<issue_to_address>
**suggestion:** Factor out duplicated `rhel_version` loop and consider using numeric types for `major`/`minor` comparisons.
The loop building `expr` is duplicated across the `UNLEASH` branches; consider extracting it into a helper that returns `expr` so both branches share the same logic. Also, in the new branch `OperatingSystem.major`/`minor` (integers) are compared to string values from `version.split('.')`. Converting these components to integers (with basic error handling) before use would avoid relying on implicit casts and make the query’s intent and types clearer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "updated": SQL("updated"), | ||
| "first_reported": SQL("first_reported"), | ||
| "os": OS_INFO_SORT, | ||
| "os": ([SQL("os")] if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT) else OS_INFO_SORT), |
There was a problem hiding this comment.
suggestion (bug_risk): Use OperatingSystem.sort_columns() when Unleash is enabled instead of [SQL("os")] to keep stable sort semantics.
This change switches the os sort key from the structured OS_INFO_SORT to the derived string column SQL("os") when Unleash is enabled, which can change ordering semantics (e.g., string vs numeric comparison, NULL handling) and tie sorting to the display format.
Since you added OperatingSystem.sort_columns(), you can use it here to keep the sort based on the underlying OS structure:
"os": (
OperatingSystem.sort_columns()
if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT)
else OS_INFO_SORT
),
Suggested change
| "os": ([SQL("os")] if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT) else OS_INFO_SORT), | |
| "os": ( | |
| OperatingSystem.sort_columns() | |
| if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT) | |
| else OS_INFO_SORT | |
| ), |
Contributor
Author
There was a problem hiding this comment.
No. The outer columns from query (in this case os) will be present and it can be referenced, but not tables such as OperatingSystem that exist only inside the query (inside the CTEs). What do you think?
manager/filters.py
Outdated
Comment on lines
458
to
467
| if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT): | ||
| for version in args["rhel_version"]: | ||
| if "." in version: | ||
| major, minor = version.split(".", 1) | ||
| expr = expr | \ | ||
| ((OperatingSystem.major == major) & | ||
| (OperatingSystem.minor == minor)) | ||
| else: | ||
| expr = expr | (OperatingSystem.major == version) | ||
| query = query.where((OperatingSystem.name == "RHEL") & (OperatingSystem.lifecycle_phase == "minor") & (expr)) |
There was a problem hiding this comment.
suggestion: Factor out duplicated rhel_version loop and consider using numeric types for major/minor comparisons.
The loop building expr is duplicated across the UNLEASH branches; consider extracting it into a helper that returns expr so both branches share the same logic. Also, in the new branch OperatingSystem.major/minor (integers) are compared to string values from version.split('.'). Converting these components to integers (with basic error handling) before use would avoid relying on implicit casts and make the query’s intent and types clearer.
Cyndi migration inventory hosts system profile os to operating system RHINENG-23544
jdobes
requested changes
Feb 26, 2026
|
|
||
| if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT): | ||
| query = (query.switch(SystemPlatform).join(SystemGroupSet, JOIN.LEFT_OUTER)) | ||
| query = (query.switch(SystemPlatform).join(OperatingSystem, JOIN.LEFT_OUTER, |
Member
There was a problem hiding this comment.
The join with OperatingSystem table shouldn't be done here. This is a place where permissions to see workspaces/groups are handled.
| continue | ||
| expr = cond if expr is None else expr | cond | ||
| if expr is not None: | ||
| query = query.where((OperatingSystem.name == "RHEL") & (OperatingSystem.lifecycle_phase == "minor") & (expr)) |
Member
There was a problem hiding this comment.
The lifecycle_phase shouldn't be restricted to minor
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
2 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.
Cyndi migration inventory hosts system profile os to operating system RHINENG-23544
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Integrate the dedicated operating_system table into queries, filtering, and development data to support the Cyndi replication replacement feature flag and improve OS display and sorting.
New Features:
Enhancements:
Chores: