-
Notifications
You must be signed in to change notification settings - Fork 37
RHINENG-21823: Remove receptor code from getPlaybookRuns #872
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 GuideRefactor getPlaybookRuns to remove receptor-specific joins and data, initialize executors in combineRuns, and adjust integration tests to expect empty or simplified executor arrays. Sequence diagram for combineRuns executor initialization and populationsequenceDiagram
participant combineRuns
participant PlaybookRun
participant playbook-dispatcher
combineRuns->>PlaybookRun: Check if executors exists
alt executors not present
combineRuns->>PlaybookRun: Initialize executors as []
end
combineRuns->>playbook-dispatcher: Fetch run details for PlaybookRun.id
playbook-dispatcher-->>combineRuns: Return direct/satellite host executor data
combineRuns->>PlaybookRun: Populate executors with dispatcher data
Class diagram for updated PlaybookRun and Executor data structureclassDiagram
class Remediation {
+id
+tenant_org_id
+created_by
+playbook_runs: PlaybookRun[]
}
class PlaybookRun {
+id
+executors: Executor[]
}
class Executor {
+executor_id
+executor_name
+status
+system_count
+counts
}
Remediation "1" -- "*" PlaybookRun : has
PlaybookRun "1" -- "*" Executor : has
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:
- Consider assessing the performance impact of replacing the aggregated database executor counts with per-run calls to the playbook-dispatcher API, as this could introduce additional latency under load.
- Since getPlaybookRuns now always returns an empty executors array, you could move that default initialization into the query layer to simplify combineRuns and ensure consistency.
- There are many repetitive
.executors.should.have.length(0)assertions in the integration tests—extract a helper or shared assertion to DRY up those checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider assessing the performance impact of replacing the aggregated database executor counts with per-run calls to the playbook-dispatcher API, as this could introduce additional latency under load.
- Since getPlaybookRuns now always returns an empty executors array, you could move that default initialization into the query layer to simplify combineRuns and ensure consistency.
- There are many repetitive `.executors.should.have.length(0)` assertions in the integration tests—extract a helper or shared assertion to DRY up those checks.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return result; | ||
| }; | ||
|
|
||
| // For now, we're returning an empty executors array because we no longer use playbook_run_executors or playbook_run_systems tables (receptor data) |
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.
Something that might be useful for a lot of the queries that we're cleaning up...
We wouldn't need to make any API calls in formatRHCRuns for Direct or Satellite cases if we had a dispatcher_runs_hosts table. dispatcher_runs_hosts would store host-level execution status and counts that we get from Kafka Run Host events from playbook-dispatcher..
The dispatcher_runs table helps in a lot of cases but for endpoints like GET /remediations/id/playbook_runs, we need the host-level run data
Summary by Sourcery
Remove direct database receptor joins from getPlaybookRuns, return empty executor arrays by default, and shift executor population to the combineRuns function while updating tests and snapshots accordingly.
Enhancements:
Tests: