-
Notifications
You must be signed in to change notification settings - Fork 37
RHINENG-22115: Filter out invalid remediations/issues in resolveResol… #894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors resolveResolutionsNeedReboot to ignore invalid remediation objects, pre-filter issues before processing, and streamline the reboot-resolution mapping logic while preserving behavior of encoding reboot status into the resolution field. Sequence diagram for resolveResolutionsNeedReboot processing valid issuessequenceDiagram
participant Caller
participant resolveResolutionsNeedReboot
participant Lodash
participant Identifiers
participant Issues
participant ResolutionResolver
participant PromiseAll
Caller->>resolveResolutionsNeedReboot: call with remediations
activate resolveResolutionsNeedReboot
resolveResolutionsNeedReboot->>resolveResolutionsNeedReboot: filter invalid remediations
resolveResolutionsNeedReboot->>Lodash: flatMap issues from validRemediations
Lodash-->>resolveResolutionsNeedReboot: allIssues array
resolveResolutionsNeedReboot->>PromiseAll: map allIssues to async tasks
activate PromiseAll
loop for each issue in allIssues
PromiseAll->>Identifiers: parse(issue.issue_id)
Identifiers-->>PromiseAll: id
PromiseAll->>Issues: getHandler(id)
Issues-->>PromiseAll: handler
PromiseAll->>ResolutionResolver: handler.getResolutionResolver()
ResolutionResolver-->>PromiseAll: resolver
PromiseAll->>ResolutionResolver: isRebootNeeded(id, issue.resolution)
ResolutionResolver-->>PromiseAll: needsReboot (boolean or null)
alt needsReboot is not null
PromiseAll->>PromiseAll: issue.resolution = { needsReboot }
else needsReboot is null
PromiseAll->>PromiseAll: issue.resolution = false
end
end
PromiseAll-->>resolveResolutionsNeedReboot: all async tasks resolved
deactivate PromiseAll
resolveResolutionsNeedReboot-->>Caller: resolved Promise
deactivate resolveResolutionsNeedReboot
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- By filtering out invalid remediations instead of logging them, the function now silently ignores bad inputs; if this change is intentional, consider at least a single aggregated log or metric to preserve observability of malformed remediations.
- Since
issue.resolutionis being overloaded from its original string meaning into either a boolean or an object, consider extracting this into a small helper (e.g.,setRebootResolution(issue, needsReboot)) to centralize and clarify this unconventional usage and make future refactors easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By filtering out invalid remediations instead of logging them, the function now silently ignores bad inputs; if this change is intentional, consider at least a single aggregated log or metric to preserve observability of malformed remediations.
- Since `issue.resolution` is being overloaded from its original string meaning into either a boolean or an object, consider extracting this into a small helper (e.g., `setRebootResolution(issue, needsReboot)`) to centralize and clarify this unconventional usage and make future refactors easier.
## Individual Comments
### Comment 1
<location> `src/remediations/controller.read.js:49-53` </location>
<code_context>
- log.warn({ index: i, remediation: r }, "resolveResolutionsNeedReboot: remediation is missing 'issues' array");
- }
- });
+ // Filter out invalid remediations (null/undefined or missing issues array)
+ const validRemediations = remediations.filter(r => r && Array.isArray(r.issues));
+
+ // Collect all issues from all remediations
</code_context>
<issue_to_address>
**suggestion:** Consider retaining some observability when remediations are filtered out as invalid.
Previously, invalid remediations produced warnings that helped detect upstream data issues. Now they’re silently dropped. If this behavior is intentional, consider logging when `remediations.length !== validRemediations.length` (or at least the number filtered out) so unexpected input shapes remain visible without cluttering the main logic.
```suggestion
function resolveResolutionsNeedReboot (...remediations) {
// Filter out invalid remediations (null/undefined or missing issues array)
const validRemediations = remediations.filter(r => r && Array.isArray(r.issues));
// Retain observability: log when some remediations are filtered out as invalid
if (remediations.length !== validRemediations.length) {
const filteredOut = remediations.length - validRemediations.length;
log.warn(
{
totalRemediations: remediations.length,
validRemediations: validRemediations.length,
filteredOut
},
'resolveResolutionsNeedReboot: filtered out invalid remediations (null/undefined or missing issues array)'
);
}
// Collect all issues from all remediations
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…utionsNeedReboot
Summary by Sourcery
Filter and process only valid remediation issues when resolving whether resolutions need a reboot.
Bug Fixes:
Enhancements: