Fixes #39185 - Track errata applications in database#11690
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds persistent errata application tracking: a new Changes
Sequence Diagram(s)sequenceDiagram
participant Task as ForemanTasks::Task
participant Middleware as Actions::Middleware::RecordErrataApplication
participant Finder as Katello::ErrataApplication (helpers)
participant Host as Host::Managed
participant Erratum as Katello::Erratum
participant DB as Database
Task->>Middleware: finalize(event)
Middleware->>Middleware: resolve task by execution_plan_id / external_id
Middleware->>Finder: errata_install_job?(task, action)
alt RemoteExecution::RunHostJob
Finder->>Task: inspect template_invocation inputs for 'errata'/'Errata search query'
else Other action types
Finder->>Task: inspect action.label against allowlist
end
alt Task is errata job
Middleware->>Finder: extract_host_id_from_task(task)
Middleware->>Finder: extract_errata_ids_from_task(task)
Middleware->>Host: load host by id
Middleware->>Erratum: load errata by ids
Middleware->>Finder: determine_status(task, action)
Middleware->>Finder: determine_method_from_task(task)
loop per erratum id
Middleware->>DB: create Katello::ErrataApplication record
DB-->>Middleware: created / skipped (duplicate)
end
else Not errata
Middleware-->>Task: no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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 5 issues, and left some high level feedback:
- In the migration you declare
task_idas astringwhile the model usesbelongs_to :task, class_name: 'ForemanTasks::Task', which expects an integer foreign key; consider switching this to a propert.references :task(with the correct table and type) or updating the association to match the column type. ErrataApplication.record_from_taskcurrently records for tasks in statesrunningandstopped; if you only want completed applications, narrowing this to the final/terminal states (e.g. juststopped) would avoid persisting partial or in-progress runs.- When rescuing
ActiveRecord::RecordInvalidinErrataApplication.record_from_task, the log message only mentions the erratum and host; including the task ID and validation errors in the warning would make troubleshooting skipped records significantly easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the migration you declare `task_id` as a `string` while the model uses `belongs_to :task, class_name: 'ForemanTasks::Task'`, which expects an integer foreign key; consider switching this to a proper `t.references :task` (with the correct table and type) or updating the association to match the column type.
- `ErrataApplication.record_from_task` currently records for tasks in states `running` and `stopped`; if you only want completed applications, narrowing this to the final/terminal states (e.g. just `stopped`) would avoid persisting partial or in-progress runs.
- When rescuing `ActiveRecord::RecordInvalid` in `ErrataApplication.record_from_task`, the log message only mentions the erratum and host; including the task ID and validation errors in the warning would make troubleshooting skipped records significantly easier.
## Individual Comments
### Comment 1
<location path="app/models/katello/errata_application.rb" line_range="5" />
<code_context>
+ class ErrataApplication < Katello::Model
+ belongs_to :host, class_name: '::Host::Managed', inverse_of: :errata_applications
+ belongs_to :erratum, class_name: 'Katello::Erratum', inverse_of: :errata_applications
+ belongs_to :task, class_name: 'ForemanTasks::Task', optional: true, inverse_of: false
+ belongs_to :user, class_name: '::User', optional: true
+
</code_context>
<issue_to_address>
**issue (bug_risk):** The `task` association does not match the schema and will point to the wrong key by default.
The migration defines `task_id` as a `string`, which likely stores `ForemanTasks::Task.external_id`, but `belongs_to :task` assumes an integer `task_id` referencing `ForemanTasks::Task.id`. This will break calls like `errata_application.task`. Please either configure the association:
```ruby
depends_to :task,
class_name: 'ForemanTasks::Task',
primary_key: :external_id,
foreign_key: :task_id,
optional: true,
inverse_of: false
```
or change `task_id` to be a proper foreign key to `foreman_tasks_tasks.id`, depending on the intended relationship.
</issue_to_address>
### Comment 2
<location path="app/models/katello/errata_application.rb" line_range="22" />
<code_context>
+
+ default_scope { order(applied_at: :desc) }
+
+ validates :erratum_id, uniqueness: { scope: [:host_id, :applied_at] }
+
+ # Record errata applications from a completed task
</code_context>
<issue_to_address>
**issue (bug_risk):** The uniqueness validation is not backed by a unique index, which can allow duplicates under concurrency.
The model validates `erratum_id` uniqueness with scope `[:host_id, :applied_at]`, but the DB index on `[:host_id, :erratum_id, :applied_at]` is not unique. Under concurrent inserts, this can still create duplicates. Please change this to a unique index:
```ruby
add_index :katello_errata_applications,
[:host_id, :erratum_id, :applied_at],
unique: true,
name: 'index_errata_apps_host_erratum_date'
```
and update the migration so the database enforces the constraint.
</issue_to_address>
### Comment 3
<location path="app/models/katello/errata_application.rb" line_range="30" />
<code_context>
+ # @return [Array<ErrataApplication>] Created application records
+ def self.record_from_task(task, action = nil)
+ return [] unless task
+ return [] unless task.state.in?(%w[running stopped])
+
+ host_id = extract_host_id_from_task(task)
</code_context>
<issue_to_address>
**issue (bug_risk):** The state filter in `record_from_task` seems inverted relative to the intention to handle completed tasks.
The docstring states this should run for completed tasks, but the guard only allows `running` or `stopped` and returns early for everything else. That both permits processing while still `running` and excludes other terminal states (e.g. `paused`, or future ones). If this is meant for finished tasks only, consider checking explicitly for terminal state(s), e.g.:
```ruby
return [] unless task.state == 'stopped'
```
or using a whitelist of terminal states instead of including `running`.
</issue_to_address>
### Comment 4
<location path="test/lib/concerns/base_template_scope_extensions_test.rb" line_range="24" />
<code_context>
assert_equal @errata.id.to_s, id
end
+
+ def test_load_errata_applications_returns_array
+ create_errata_application(@host, @errata)
+ scope = ::Foreman::Renderer.get_scope
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the legacy `load_errata_applications_legacy` path when the table does not exist
The new tests only cover the code path where `Katello::ErrataApplication.table_exists?` is true, so the legacy task-parsing path is now untested. Please add a test that stubs `Katello::ErrataApplication.table_exists?` to `false` and verifies that `load_errata_applications` delegates to `load_errata_applications_legacy` (e.g., by stubbing that method and asserting it was called, or by checking the returned data matches the legacy structure).
</issue_to_address>
### Comment 5
<location path="test/models/katello/errata_application_test.rb" line_range="228" />
<code_context>
+ refute_includes applications, manual_app
+ end
+
+ def test_record_from_task_with_valid_task
+ task = create_task_with_errata(@host, [@erratum.errata_id])
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for additional `record_from_task` extraction and method/status branches
Current tests exercise the main `record_from_task` flow and some guards, but several branches in `ErrataApplication` remain untested:
- `extract_errata_ids_from_task` when IDs come from `input['content']`.
- Script-based extraction (`extract_from_script`) for jobs using `job_features` and `RESOLVED_ERRATA_IDS` in the script.
- Template-input-based extraction (`extract_from_template_input`) when errata are passed as template input values.
- The default branch of `determine_method_from_task` (neither REX nor Katello agent), including verifying that multiple errata IDs in one task yield multiple application records.
Targeted tests for these paths would better protect the parsing logic that populates the tracking table.
Suggested implementation:
```ruby
class ErrataApplicationTest < ActiveSupport::TestCase
def setup
@host = hosts(:one)
@erratum = katello_errata(:security)
@user = User.first
end
def test_extract_errata_ids_from_task_from_content
task = {
'input' => {
'content' => {
'errata' => [@erratum.errata_id, 'RHBA-2024:0001']
}
}
}
ids = ErrataApplication.send(:extract_errata_ids_from_task, task)
assert_equal [@erratum.errata_id, 'RHBA-2024:0001'], ids
end
def test_extract_errata_ids_from_task_from_script
script = <<~SCRIPT
#!/bin/bash
RESOLVED_ERRATA_IDS="#{@erratum.errata_id},RHBA-2024:0002"
echo "Applying errata"
SCRIPT
task = {
'input' => {
'job_features' => ['katello_errata_install'],
'script' => script
}
}
ids = ErrataApplication.send(:extract_errata_ids_from_task, task)
assert_includes ids, @erratum.errata_id
assert_includes ids, 'RHBA-2024:0002'
end
def test_extract_errata_ids_from_task_from_template_input
task = {
'input' => {
'template_input' => {
'errata' => "#{@erratum.errata_id},RHBA-2024:0003"
}
}
}
ids = ErrataApplication.send(:extract_errata_ids_from_task, task)
assert_equal [@erratum.errata_id, 'RHBA-2024:0003'], ids
end
def test_determine_method_from_task_default_branch
task = {
'input' => {
'some' => 'payload'
}
}
method = ErrataApplication.send(:determine_method_from_task, task)
refute_equal 'remote_execution', method
refute_equal 'katello_agent', method
assert method.present?
end
def test_record_from_task_creates_multiple_applications_for_multiple_errata_ids
task = {
'id' => SecureRandom.uuid,
'input' => {
'content' => {
'errata' => [@erratum.errata_id, 'RHBA-2024:0004']
}
}
}
created = []
ErrataApplication.stub(:create!, ->(attrs) { created << attrs }) do
ErrataApplication.record_from_task(task, @host, @user)
end
assert_equal 2, created.size
created.each do |attrs|
assert_equal @host, attrs[:host]
assert_equal @user, attrs[:user]
assert_equal 'success', attrs[:status]
end
end
def test_create_valid_application
```
These tests assume the following about the implementation:
1. `extract_errata_ids_from_task`, `extract_from_script`, and `extract_from_template_input` are private class methods on `ErrataApplication` invoked by `extract_errata_ids_from_task(task)` and accept a `Hash`-like `task` with an `'input'` key.
2. The script-based extraction looks for `RESOLVED_ERRATA_IDS` in the `input['script']` value and splits it into individual errata IDs.
3. Template-input-based extraction expects `input['template_input']['errata']` as a comma-separated string of errata IDs.
4. `determine_method_from_task` accepts the same Hash-like `task` structure and returns a non-empty string; the default branch is any case where it does not return `'remote_execution'` or `'katello_agent'`.
5. `record_from_task` accepts `(task, host, user)` where `task` can be a Hash with at least `'id'` and `'input'` keys, uses `extract_errata_ids_from_task` internally, and calls `create!` once per erratum ID with `:host`, `:user`, and `:status` in the attributes.
If your implementation differs, you should adjust:
- The shape of the `task` Hashes to match the real task object (for example, using a Dynflow task or a Struct that responds to `input`, etc.).
- The keys used in `input` (`'content'`, `'template_input'`, `'job_features'`, `'script'`) to align with how the extraction methods actually read data.
- The `record_from_task` call signature and the expectations on the attributes passed to `create!` (e.g., `:host_id` instead of `:host`, different `:status` values, or additional required keys like `:method` or `:applied_at`).
You may also want to:
- Replace the direct `send(:extract_errata_ids_from_task, task)` calls with tests that go through `record_from_task` if those helpers are not intended to be part of the test surface.
- Use existing task factories or helper methods (e.g. `create_task_with_errata`) if they already encapsulate the correct Dynflow task structure, adapting these tests to build on those helpers instead of raw Hashes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| class ErrataApplication < Katello::Model | ||
| belongs_to :host, class_name: '::Host::Managed', inverse_of: :errata_applications | ||
| belongs_to :erratum, class_name: 'Katello::Erratum', inverse_of: :errata_applications | ||
| belongs_to :task, class_name: 'ForemanTasks::Task', optional: true, inverse_of: false |
There was a problem hiding this comment.
issue (bug_risk): The task association does not match the schema and will point to the wrong key by default.
The migration defines task_id as a string, which likely stores ForemanTasks::Task.external_id, but belongs_to :task assumes an integer task_id referencing ForemanTasks::Task.id. This will break calls like errata_application.task. Please either configure the association:
depends_to :task,
class_name: 'ForemanTasks::Task',
primary_key: :external_id,
foreign_key: :task_id,
optional: true,
inverse_of: falseor change task_id to be a proper foreign key to foreman_tasks_tasks.id, depending on the intended relationship.
| refute_includes applications, manual_app | ||
| end | ||
|
|
||
| def test_record_from_task_with_valid_task |
There was a problem hiding this comment.
suggestion (testing): Add tests for additional record_from_task extraction and method/status branches
Current tests exercise the main record_from_task flow and some guards, but several branches in ErrataApplication remain untested:
extract_errata_ids_from_taskwhen IDs come frominput['content'].- Script-based extraction (
extract_from_script) for jobs usingjob_featuresandRESOLVED_ERRATA_IDSin the script. - Template-input-based extraction (
extract_from_template_input) when errata are passed as template input values. - The default branch of
determine_method_from_task(neither REX nor Katello agent), including verifying that multiple errata IDs in one task yield multiple application records.
Targeted tests for these paths would better protect the parsing logic that populates the tracking table.
Suggested implementation:
class ErrataApplicationTest < ActiveSupport::TestCase
def setup
@host = hosts(:one)
@erratum = katello_errata(:security)
@user = User.first
end
def test_extract_errata_ids_from_task_from_content
task = {
'input' => {
'content' => {
'errata' => [@erratum.errata_id, 'RHBA-2024:0001']
}
}
}
ids = ErrataApplication.send(:extract_errata_ids_from_task, task)
assert_equal [@erratum.errata_id, 'RHBA-2024:0001'], ids
end
def test_extract_errata_ids_from_task_from_script
script = <<~SCRIPT
#!/bin/bash
RESOLVED_ERRATA_IDS="#{@erratum.errata_id},RHBA-2024:0002"
echo "Applying errata"
SCRIPT
task = {
'input' => {
'job_features' => ['katello_errata_install'],
'script' => script
}
}
ids = ErrataApplication.send(:extract_errata_ids_from_task, task)
assert_includes ids, @erratum.errata_id
assert_includes ids, 'RHBA-2024:0002'
end
def test_extract_errata_ids_from_task_from_template_input
task = {
'input' => {
'template_input' => {
'errata' => "#{@erratum.errata_id},RHBA-2024:0003"
}
}
}
ids = ErrataApplication.send(:extract_errata_ids_from_task, task)
assert_equal [@erratum.errata_id, 'RHBA-2024:0003'], ids
end
def test_determine_method_from_task_default_branch
task = {
'input' => {
'some' => 'payload'
}
}
method = ErrataApplication.send(:determine_method_from_task, task)
refute_equal 'remote_execution', method
refute_equal 'katello_agent', method
assert method.present?
end
def test_record_from_task_creates_multiple_applications_for_multiple_errata_ids
task = {
'id' => SecureRandom.uuid,
'input' => {
'content' => {
'errata' => [@erratum.errata_id, 'RHBA-2024:0004']
}
}
}
created = []
ErrataApplication.stub(:create!, ->(attrs) { created << attrs }) do
ErrataApplication.record_from_task(task, @host, @user)
end
assert_equal 2, created.size
created.each do |attrs|
assert_equal @host, attrs[:host]
assert_equal @user, attrs[:user]
assert_equal 'success', attrs[:status]
end
end
def test_create_valid_applicationThese tests assume the following about the implementation:
extract_errata_ids_from_task,extract_from_script, andextract_from_template_inputare private class methods onErrataApplicationinvoked byextract_errata_ids_from_task(task)and accept aHash-liketaskwith an'input'key.- The script-based extraction looks for
RESOLVED_ERRATA_IDSin theinput['script']value and splits it into individual errata IDs. - Template-input-based extraction expects
input['template_input']['errata']as a comma-separated string of errata IDs. determine_method_from_taskaccepts the same Hash-liketaskstructure and returns a non-empty string; the default branch is any case where it does not return'remote_execution'or'katello_agent'.record_from_taskaccepts(task, host, user)wheretaskcan be a Hash with at least'id'and'input'keys, usesextract_errata_ids_from_taskinternally, and callscreate!once per erratum ID with:host,:user, and:statusin the attributes.
If your implementation differs, you should adjust:
- The shape of the
taskHashes to match the real task object (for example, using a Dynflow task or a Struct that responds toinput, etc.). - The keys used in
input('content','template_input','job_features','script') to align with how the extraction methods actually read data. - The
record_from_taskcall signature and the expectations on the attributes passed tocreate!(e.g.,:host_idinstead of:host, different:statusvalues, or additional required keys like:methodor:applied_at).
You may also want to:
- Replace the direct
send(:extract_errata_ids_from_task, task)calls with tests that go throughrecord_from_taskif those helpers are not intended to be part of the test surface. - Use existing task factories or helper methods (e.g.
create_task_with_errata) if they already encapsulate the correct Dynflow task structure, adapting these tests to build on those helpers instead of raw Hashes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
app/lib/katello/concerns/base_template_scope_extensions.rb (2)
248-251: Re-enable the other metrics after the legacy helper.Line 325 only turns
Metrics/MethodLengthback on.Metrics/AbcSize,Metrics/CyclomaticComplexity, andMetrics/PerceivedComplexitystay disabled until Line 415, which widens the suppression beyondload_errata_applications_legacy.Suggested fix
result end # rubocop:enable Metrics/MethodLength + # rubocop:enable Metrics/AbcSize + # rubocop:enable Metrics/CyclomaticComplexity + # rubocop:enable Metrics/PerceivedComplexityAlso applies to: 325-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/katello/concerns/base_template_scope_extensions.rb` around lines 248 - 251, The RuboCop disables for Metrics/AbcSize, Metrics/CyclomaticComplexity, and Metrics/PerceivedComplexity were not re-enabled after the legacy helper; locate the legacy helper method load_errata_applications_legacy and where Metrics/MethodLength is turned back on and add corresponding rubocop:enable lines for Metrics/AbcSize, Metrics/CyclomaticComplexity, and Metrics/PerceivedComplexity so all four rules are re-enabled immediately after that helper (ensuring the suppression scope only covers the legacy helper).
347-357: Keep the DB-only helper internal unless you want to support it as public API.Adding Apipie docs here exposes
load_errata_applications_from_dbas a template macro, but unlikeload_errata_applicationsit has notable_exists?guard. That lets callers bypass the compatibility wrapper and hit the DB-only path directly on systems where the migration is not finished yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/katello/concerns/base_template_scope_extensions.rb` around lines 347 - 357, The apipie doc block currently exposes the DB-only helper load_errata_applications_from_db as a public template macro; remove or hide that apipie documentation so callers cannot invoke the DB-only path directly, or alternatively add the same table existence guard used by load_errata_applications (call table_exists? before touching DB and raise/return fallback when false) to load_errata_applications_from_db to prevent bypassing the compatibility wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/katello/concerns/base_template_scope_extensions.rb`:
- Around line 369-387: The DB-backed branch using Katello::ErrataApplication
bypasses the prior permission check that load_resource(..., permission:
'view_foreman_tasks') enforced; restore equivalent authorization by applying the
same readable/authorized scope before further filtering (e.g. use the same
permission symbol 'view_foreman_tasks' and the same policy/scope helper used by
load_resource) to the Katello::ErrataApplication relation so the subsequent
where/since/up_to/host_id filters only operate on records the caller is allowed
to view.
In `@app/models/katello/errata_application.rb`:
- Around line 58-60: The current rescue in ErrataApplication that rescues
ActiveRecord::RecordInvalid and assumes every validation failure is a duplicate
should be tightened: in the rescue for ActiveRecord::RecordInvalid (the block
that logs "Skipped duplicate: #{erratum.errata_id} on #{host.name}"), inspect
the exception.record.errors (or errors.details) to determine if the failure is
actually the uniqueness violation for the errata/host combination (e.g., error
on :errata_id or :host_id with :taken/has already been taken), and only
swallow/log as a duplicate in that case; for any other validation errors
re-raise the exception (or log full validation errors) so real data problems
aren't silently lost. Ensure the rescue references ActiveRecord::RecordInvalid
and uses exception.record.errors to distinguish uniqueness errors from other
validation failures.
- Around line 95-99: The code in extract_from_script relies on
execution_plan.actions[1] which is brittle; replace that hardcoded index by
scanning execution_plan.all_planned_actions for the action that contains a
script (e.g., action.try(:input).try(:[], 'script')). In extract_from_script,
iterate over execution_plan.all_planned_actions, pull the first non-nil script
string (or concatenate/scan them) and then search its lines for the '#
RESOLVED_ERRATA_IDS=' marker, preserving the existing parsing logic (split on
'=', split by ',' and strip/reject blank). Apply the same change to the other
occurrence mentioned (base_template_scope_extensions.rb) so both use
all_planned_actions rather than actions[1].
In `@db/migrate/20260327192122_create_katello_errata_applications.rb`:
- Line 17: The non-unique index index_errata_apps_host_erratum_date allows
duplicate rows under concurrent writes; change the migration to enforce DB-level
uniqueness by creating a unique constraint/index on [:host_id, :erratum_id,
:applied_at] (e.g., add or replace the existing t.index call with a unique index
or use add_index with unique: true) so the database rejects duplicates and
matches any application-level validation in the Katello::ErrataApplications
table; ensure the index name remains 'index_errata_apps_host_erratum_date' (or
drop the old non-unique index first if needed) to avoid duplicate-index
conflicts during migration.
---
Nitpick comments:
In `@app/lib/katello/concerns/base_template_scope_extensions.rb`:
- Around line 248-251: The RuboCop disables for Metrics/AbcSize,
Metrics/CyclomaticComplexity, and Metrics/PerceivedComplexity were not
re-enabled after the legacy helper; locate the legacy helper method
load_errata_applications_legacy and where Metrics/MethodLength is turned back on
and add corresponding rubocop:enable lines for Metrics/AbcSize,
Metrics/CyclomaticComplexity, and Metrics/PerceivedComplexity so all four rules
are re-enabled immediately after that helper (ensuring the suppression scope
only covers the legacy helper).
- Around line 347-357: The apipie doc block currently exposes the DB-only helper
load_errata_applications_from_db as a public template macro; remove or hide that
apipie documentation so callers cannot invoke the DB-only path directly, or
alternatively add the same table existence guard used by
load_errata_applications (call table_exists? before touching DB and raise/return
fallback when false) to load_errata_applications_from_db to prevent bypassing
the compatibility wrapper.
🪄 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: Pro
Run ID: 98fce92f-061d-4dd5-a5de-7018f6f4fd43
📥 Commits
Reviewing files that changed from the base of the PR and between 2b1e044 and 2f94da17745777de913e664b74039a038c31ded7.
📒 Files selected for processing (9)
app/lib/actions/middleware/record_errata_application.rbapp/lib/katello/concerns/base_template_scope_extensions.rbapp/models/katello/concerns/host_managed_extensions.rbapp/models/katello/errata_application.rbapp/models/katello/erratum.rbdb/migrate/20260327192122_create_katello_errata_applications.rblib/katello/engine.rbtest/lib/concerns/base_template_scope_extensions_test.rbtest/models/katello/errata_application_test.rb
pavanshekar
force-pushed
the
issue-39185
branch
from
2f94da1 to
4033cc9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/lib/concerns/base_template_scope_extensions_test.rb (1)
8-10: Consider using a fixture instead ofUser.firstfor reliability.
User.firstmay returnnilif the users table is empty or if the database order changes. Using a named fixture (likeusers(:admin)or similar) would be more reliable and explicit.Proposed fix
`@host` = hosts(:one) `@errata` = katello_errata(:security) `@bugfix` = katello_errata(:bugfix) - `@user` = User.first + `@user` = users(:admin) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/lib/concerns/base_template_scope_extensions_test.rb` around lines 8 - 10, Replace the brittle User.first usage in the test setup with a named fixture: change the `@user` assignment (in the setup for BaseTemplateScopeExtensionsTest) from User.first to a specific fixture reference such as users(:admin) or users(:one); ensure the test includes the users fixture (fixtures :users) if not already present so `@user` reliably references the intended fixture.app/models/katello/errata_application.rb (1)
20-20: Consider removingdefault_scopeto avoid unexpected query behavior.
default_scopecan cause confusion and unexpected behavior when composing queries, particularly when usingunscopedis forgotten or when the scope interferes with joins. Consider removing it and explicitly ordering where needed, or using a named scope likerecentthat can be applied intentionally.Proposed fix
- default_scope { order(applied_at: :desc) } + scope :recent, -> { order(applied_at: :desc) }Then update callers to use
.recentexplicitly where ordering is desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/katello/errata_application.rb` at line 20, Remove the model-level default_scope that orders by applied_at to avoid hidden query behavior; replace it with an explicit named scope (e.g., def self.recent or scope :recent, -> { order(applied_at: :desc) }) in the Katello::ErrataApplication model and update all call sites that rely on the implicit ordering to call .recent explicitly (search for usages of Katello::ErrataApplication, ErrataApplication, and any code that expected default ordering) so queries and joins no longer inherit the ordering unexpectedly.app/lib/katello/concerns/base_template_scope_extensions.rb (1)
396-411:find_eachmay conflict withdefault_scopeordering.
find_eachinternally uses primary key ordering for batching and ignores any customORDER BYclause. Thedefault_scope { order(applied_at: :desc) }onErrataApplicationwill be overridden byfind_each, which may lead to unexpected ordering in the results. If ordering matters for the report output, consider using.unscopedor removing thedefault_scopeas suggested earlier.Proposed fix if ordering matters
- applications.find_each(batch_size: 1000) do |app| + applications.reorder(nil).find_each(batch_size: 1000) do |app|Or if ordered output is needed, use
in_batchesinstead:applications.in_batches(of: 1000) do |batch| batch.each do |app| # ... end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/katello/concerns/base_template_scope_extensions.rb` around lines 396 - 411, The report uses applications.find_each(batch_size: 1000) which ignores the ErrataApplication default_scope ordering (order(applied_at: :desc)), causing unexpected output order; change the batching to preserve order by either calling applications.in_batches(of: 1000) and iterating each batch with batch.each (so applied_at ordering is respected) or use applications.unscoped.find_each if you intentionally want primary-key ordering, or remove the default_scope on ErrataApplication; update the loop in base_template_scope_extensions.rb where applications.find_each is used to one of these options to ensure the desired ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/lib/katello/concerns/base_template_scope_extensions.rb`:
- Around line 396-411: The report uses applications.find_each(batch_size: 1000)
which ignores the ErrataApplication default_scope ordering (order(applied_at:
:desc)), causing unexpected output order; change the batching to preserve order
by either calling applications.in_batches(of: 1000) and iterating each batch
with batch.each (so applied_at ordering is respected) or use
applications.unscoped.find_each if you intentionally want primary-key ordering,
or remove the default_scope on ErrataApplication; update the loop in
base_template_scope_extensions.rb where applications.find_each is used to one of
these options to ensure the desired ordering.
In `@app/models/katello/errata_application.rb`:
- Line 20: Remove the model-level default_scope that orders by applied_at to
avoid hidden query behavior; replace it with an explicit named scope (e.g., def
self.recent or scope :recent, -> { order(applied_at: :desc) }) in the
Katello::ErrataApplication model and update all call sites that rely on the
implicit ordering to call .recent explicitly (search for usages of
Katello::ErrataApplication, ErrataApplication, and any code that expected
default ordering) so queries and joins no longer inherit the ordering
unexpectedly.
In `@test/lib/concerns/base_template_scope_extensions_test.rb`:
- Around line 8-10: Replace the brittle User.first usage in the test setup with
a named fixture: change the `@user` assignment (in the setup for
BaseTemplateScopeExtensionsTest) from User.first to a specific fixture reference
such as users(:admin) or users(:one); ensure the test includes the users fixture
(fixtures :users) if not already present so `@user` reliably references the
intended fixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1260da11-d78d-4166-8a1b-e8d56fd1b2ae
📥 Commits
Reviewing files that changed from the base of the PR and between 2f94da17745777de913e664b74039a038c31ded7 and 4033cc93f9a804b6707ab2c7d3580ba07ee871a5.
📒 Files selected for processing (9)
app/lib/actions/middleware/record_errata_application.rbapp/lib/katello/concerns/base_template_scope_extensions.rbapp/models/katello/concerns/host_managed_extensions.rbapp/models/katello/errata_application.rbapp/models/katello/erratum.rbdb/migrate/20260327192122_create_katello_errata_applications.rblib/katello/engine.rbtest/lib/concerns/base_template_scope_extensions_test.rbtest/models/katello/errata_application_test.rb
✅ Files skipped from review due to trivial changes (1)
- db/migrate/20260327192122_create_katello_errata_applications.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- test/models/katello/errata_application_test.rb
pavanshekar
force-pushed
the
issue-39185
branch
from
4033cc9 to
875b9a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/actions/middleware/record_errata_application.rb`:
- Around line 32-43: The non-REX branch of errata_install_job? currently only
checks for action.label including 'Actions::Katello::Host::Erratum::Install',
which misses legacy ApplicableErrataInstall jobs; update the check in
errata_install_job? (the method in record_errata_application.rb) to also match
'Actions::Katello::Host::Erratum::ApplicableErrataInstall' (or more robustly
match the common namespace 'Actions::Katello::Host::Erratum::') so both Install
and ApplicableErrataInstall are treated as errata-install tasks, ensuring parity
with the legacy/template logic that references ApplicableErrataInstall.
In `@app/lib/katello/concerns/base_template_scope_extensions.rb`:
- Around line 369-389: Replace the array materialization via pluck(:id) with SQL
subqueries: for the authorized set, change the pre-check from
authorized_host_ids = ::Host::Managed.authorized('view_hosts').pluck(:id) and
return [] if authorized_host_ids.empty? to using the relation (e.g. auth_hosts =
::Host::Managed.authorized('view_hosts')) and short-circuit with return [] if
auth_hosts.none?; then apply the subquery in the ErrataApplication where clause
as applications =
Katello::ErrataApplication.includes(includes_list).where(host_id:
auth_hosts.select(:id)). Similarly, replace host_ids =
::Host.search_for(host_filter).pluck(:id) and applications =
applications.where(host_id: host_ids) with applications =
applications.where(host_id: ::Host.search_for(host_filter).select(:id)). Keep
the rest of the status/since/up_to logic 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: Pro
Run ID: 5cd54644-aea2-41d6-a46f-c3927d96f1f3
📥 Commits
Reviewing files that changed from the base of the PR and between 4033cc93f9a804b6707ab2c7d3580ba07ee871a5 and 875b9a127d1d0dc8d30e41e2fcf8ae6692392a84.
📒 Files selected for processing (9)
app/lib/actions/middleware/record_errata_application.rbapp/lib/katello/concerns/base_template_scope_extensions.rbapp/models/katello/concerns/host_managed_extensions.rbapp/models/katello/errata_application.rbapp/models/katello/erratum.rbdb/migrate/20260327192122_create_katello_errata_applications.rblib/katello/engine.rbtest/lib/concerns/base_template_scope_extensions_test.rbtest/models/katello/errata_application_test.rb
✅ Files skipped from review due to trivial changes (1)
- db/migrate/20260327192122_create_katello_errata_applications.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/katello/engine.rb
- app/models/katello/erratum.rb
- test/lib/concerns/base_template_scope_extensions_test.rb
- app/models/katello/errata_application.rb
pavanshekar
force-pushed
the
issue-39185
branch
from
875b9a1 to
63a263e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/lib/katello/concerns/base_template_scope_extensions.rb (1)
377-383: Consider handling invalid date parsing gracefully.
Time.zone.parse(since)andTime.zone.parse(up_to)will raiseArgumentErrorif the string is not a valid date format. The legacy implementation uses string interpolation in the search query which may handle this differently.Consider wrapping in error handling or validating input:
Suggested defensive parsing
if since.present? - applications = applications.since(Time.zone.parse(since)) + begin + applications = applications.since(Time.zone.parse(since)) + rescue ArgumentError + Rails.logger.warn("Invalid 'since' date format: #{since}") + end end if up_to.present? - applications = applications.up_to(Time.zone.parse(up_to)) + begin + applications = applications.up_to(Time.zone.parse(up_to)) + rescue ArgumentError + Rails.logger.warn("Invalid 'up_to' date format: #{up_to}") + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/katello/concerns/base_template_scope_extensions.rb` around lines 377 - 383, Wrap the Time.zone.parse calls for the since and up_to inputs in defensive parsing so invalid date strings don't raise ArgumentError; e.g. parse into a local variable (parsed_since = Time.zone.parse(since) rescue nil) and only call applications.since(parsed_since) if parsed_since.present? Do the same for up_to (parsed_up_to = Time.zone.parse(up_to) rescue nil) before calling applications.up_to(parsed_up_to), and optionally log or return a validation error when parsing fails. Ensure references to Time.zone.parse, the since/up_to variables, and applications.since/applications.up_to are used so you modify the correct spots.app/models/katello/errata_application.rb (1)
20-20: Consider avoidingdefault_scopefor ordering.
default_scope { order(applied_at: :desc) }can lead to unexpected behavior in queries, especially when joining with other tables or when you need a different ordering. It requires explicitunscopedorreordercalls to override.Consider using an explicit scope instead:
- default_scope { order(applied_at: :desc) } + scope :recent_first, -> { order(applied_at: :desc) }Then apply
.recent_firstexplicitly where needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/katello/errata_application.rb` at line 20, The model currently uses default_scope { order(applied_at: :desc) } which can cause surprising query behavior; replace it with an explicit scope (e.g., scope :recent_first, -> { order(applied_at: :desc) }) inside the ErrataApplication model and remove the default_scope, then update all call sites that rely on the implicit ordering to use .recent_first (or call .reorder(...) where different ordering is required) so queries and joins behave predictably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/lib/katello/concerns/base_template_scope_extensions.rb`:
- Around line 377-383: Wrap the Time.zone.parse calls for the since and up_to
inputs in defensive parsing so invalid date strings don't raise ArgumentError;
e.g. parse into a local variable (parsed_since = Time.zone.parse(since) rescue
nil) and only call applications.since(parsed_since) if parsed_since.present? Do
the same for up_to (parsed_up_to = Time.zone.parse(up_to) rescue nil) before
calling applications.up_to(parsed_up_to), and optionally log or return a
validation error when parsing fails. Ensure references to Time.zone.parse, the
since/up_to variables, and applications.since/applications.up_to are used so you
modify the correct spots.
In `@app/models/katello/errata_application.rb`:
- Line 20: The model currently uses default_scope { order(applied_at: :desc) }
which can cause surprising query behavior; replace it with an explicit scope
(e.g., scope :recent_first, -> { order(applied_at: :desc) }) inside the
ErrataApplication model and remove the default_scope, then update all call sites
that rely on the implicit ordering to use .recent_first (or call .reorder(...)
where different ordering is required) so queries and joins behave predictably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df48aa53-e428-42ca-8f01-91918ddb8d9f
📥 Commits
Reviewing files that changed from the base of the PR and between 875b9a127d1d0dc8d30e41e2fcf8ae6692392a84 and 63a263e763c175e787c0aa42bfeff9440c4037c5.
📒 Files selected for processing (9)
app/lib/actions/middleware/record_errata_application.rbapp/lib/katello/concerns/base_template_scope_extensions.rbapp/models/katello/concerns/host_managed_extensions.rbapp/models/katello/errata_application.rbapp/models/katello/erratum.rbdb/migrate/20260327192122_create_katello_errata_applications.rblib/katello/engine.rbtest/lib/concerns/base_template_scope_extensions_test.rbtest/models/katello/errata_application_test.rb
✅ Files skipped from review due to trivial changes (1)
- db/migrate/20260327192122_create_katello_errata_applications.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/katello/concerns/host_managed_extensions.rb
- test/lib/concerns/base_template_scope_extensions_test.rb
pavanshekar
force-pushed
the
issue-39185
branch
from
63a263e to
6c77a8a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
app/lib/katello/concerns/base_template_scope_extensions.rb (1)
446-453:⚠️ Potential issue | 🟠 MajorUse the ProxyAction script in the legacy search-query fallback.
This branch now reads
RunHostJob, but the resolved errata marker is extracted from::Actions::RemoteExecution::ProxyActioninapp/models/katello/errata_application.rb, Lines 102-114. Whenkatello_errata_applicationsis not available yet,katello_errata_install_by_searchtasks will resolve to[]here and disappear from the compatibility report path.Proposed fix
- action = task.execution_plan_action.all_planned_actions(::Actions::RemoteExecution::RunHostJob).first - script = action.try(:input).try(:[], 'script') || '' + proxy_action = task.execution_plan_action + .all_planned_actions(::Actions::RemoteExecution::ProxyAction) + .find { |planned_action| planned_action.try(:input).try(:[], 'script').present? } + script = proxy_action.try(:input).try(:[], 'script') || '' found = script.lines.find { |line| line.start_with? '# RESOLVED_ERRATA_IDS=' } || '' - (found.chomp.split('=', 2).last || '').split(',') + (found.chomp.split('=', 2).last || '').split(',').map(&:strip).reject(&:blank?)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/katello/concerns/base_template_scope_extensions.rb` around lines 446 - 453, In errata_ids_from_template_invocation, the fallback currently only inspects a RunHostJob action for the resolved errata marker so tasks using the legacy ProxyAction are missed; update the lookup in errata_ids_from_template_invocation to try finding a ::Actions::RemoteExecution::ProxyAction (in addition to ::Actions::RemoteExecution::RunHostJob) and read its input['script'] the same way (look for the '# RESOLVED_ERRATA_IDS=' line, split on '=', and return the CSV as before) so the legacy search-query tasks produce the same []/ids result as the code in app/models/katello/errata_application.rb expects.app/models/katello/errata_application.rb (1)
71-80:⚠️ Potential issue | 🟠 MajorNormalize
ApplicableErrataInstallinput before extracting host and errata.These helpers read
task.inputdirectly, butActions::Katello::Host::Erratum::ApplicableErrataInstallstores the real payload on its plannedInstallaction.app/lib/katello/concerns/base_template_scope_extensions.rb, Lines 430-434 already handles that task shape. Without the same normalization here, those jobs pass the middleware filter butrecord_from_taskreturns[], so nothing gets persisted.Proposed fix
+ def self.normalized_task_input(task) + return {} unless task + return task.input || {} unless task.label == 'Actions::Katello::Host::Erratum::ApplicableErrataInstall' + + install_action = task.execution_plan_action + &.all_planned_actions(::Actions::Katello::Host::Erratum::Install) + &.first + install_action&.input || {} + end + # Extract host ID from task input def self.extract_host_id_from_task(task) - input = task.input + input = normalized_task_input(task) return nil unless input if input['host'].is_a?(Hash) input['host']['id'] elsif input['host_id'] @@ # Extract errata IDs from task def self.extract_errata_ids_from_task(task) - input = task.input + input = normalized_task_input(task) return [] unless input errata = input['errata'] || input['content'] return errata if errata.present?Also applies to: 83-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/katello/errata_application.rb` around lines 71 - 80, The helpers (extract_host_id_from_task and the corresponding extract_errata_from_task) read task.input directly but must first normalize the payload the same way app/lib/katello/concerns/base_template_scope_extensions.rb does: detect when task.input contains an Action wrapper with a planned_action and replace input with that planned action's payload before extracting host or errata; update extract_host_id_from_task and the other helper to check for input['action'] && input['action']['planned_action'] (and use the planned_action payload) so they read the real Install action payload rather than the outer wrapper.
🧹 Nitpick comments (1)
test/models/katello/errata_application_test.rb (1)
228-349: Cover the planned-action and script extraction paths in this suite.These tests only hit the explicit
task.input['errata']case. OneApplicableErrataInstallcase and onekatello_errata_install_by_searchcase would lock down the two branches most likely to regress in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/katello/errata_application_test.rb` around lines 228 - 349, Add tests that exercise the planned-action and script-extraction branches by creating tasks whose input mimics the two alternative shapes: (1) a planned_actions array containing an action entry with name "ApplicableErrataInstall" and the errata ids (use a new helper like create_task_with_planned_action mirroring create_task_with_errata but stubbing task.input['planned_actions'] => [{'name' => 'ApplicableErrataInstall', 'errata' => [errata_id]}]); and (2) a script/search form for katello_errata_install_by_search (create_task_with_script_search that stubs task.input to include a script or params structure with errata IDs and uses label 'Actions::Katello::Host::Erratum::Install' or the script name 'katello_errata_install_by_search'). Ensure tests call ErrataApplication.record_from_task and assert expected applications, and stub task.user as in create_task_with_errata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/lib/katello/concerns/base_template_scope_extensions.rb`:
- Around line 446-453: In errata_ids_from_template_invocation, the fallback
currently only inspects a RunHostJob action for the resolved errata marker so
tasks using the legacy ProxyAction are missed; update the lookup in
errata_ids_from_template_invocation to try finding a
::Actions::RemoteExecution::ProxyAction (in addition to
::Actions::RemoteExecution::RunHostJob) and read its input['script'] the same
way (look for the '# RESOLVED_ERRATA_IDS=' line, split on '=', and return the
CSV as before) so the legacy search-query tasks produce the same []/ids result
as the code in app/models/katello/errata_application.rb expects.
In `@app/models/katello/errata_application.rb`:
- Around line 71-80: The helpers (extract_host_id_from_task and the
corresponding extract_errata_from_task) read task.input directly but must first
normalize the payload the same way
app/lib/katello/concerns/base_template_scope_extensions.rb does: detect when
task.input contains an Action wrapper with a planned_action and replace input
with that planned action's payload before extracting host or errata; update
extract_host_id_from_task and the other helper to check for input['action'] &&
input['action']['planned_action'] (and use the planned_action payload) so they
read the real Install action payload rather than the outer wrapper.
---
Nitpick comments:
In `@test/models/katello/errata_application_test.rb`:
- Around line 228-349: Add tests that exercise the planned-action and
script-extraction branches by creating tasks whose input mimics the two
alternative shapes: (1) a planned_actions array containing an action entry with
name "ApplicableErrataInstall" and the errata ids (use a new helper like
create_task_with_planned_action mirroring create_task_with_errata but stubbing
task.input['planned_actions'] => [{'name' => 'ApplicableErrataInstall', 'errata'
=> [errata_id]}]); and (2) a script/search form for
katello_errata_install_by_search (create_task_with_script_search that stubs
task.input to include a script or params structure with errata IDs and uses
label 'Actions::Katello::Host::Erratum::Install' or the script name
'katello_errata_install_by_search'). Ensure tests call
ErrataApplication.record_from_task and assert expected applications, and stub
task.user as in create_task_with_errata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cd723e9-9c6e-44c0-92e9-a8ec0cf9bb4a
📥 Commits
Reviewing files that changed from the base of the PR and between 63a263e763c175e787c0aa42bfeff9440c4037c5 and 6c77a8a7a2adba6a31b770427eef22fa1ddcef9e.
📒 Files selected for processing (10)
app/lib/actions/middleware/record_errata_application.rbapp/lib/katello/concerns/base_template_scope_extensions.rbapp/models/katello/concerns/host_managed_extensions.rbapp/models/katello/concerns/user_extensions.rbapp/models/katello/errata_application.rbapp/models/katello/erratum.rbdb/migrate/20260327192122_create_katello_errata_applications.rblib/katello/engine.rbtest/lib/concerns/base_template_scope_extensions_test.rbtest/models/katello/errata_application_test.rb
✅ Files skipped from review due to trivial changes (2)
- db/migrate/20260327192122_create_katello_errata_applications.rb
- test/lib/concerns/base_template_scope_extensions_test.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/katello/engine.rb
- app/models/katello/erratum.rb
- app/models/katello/concerns/host_managed_extensions.rb
|
Before jumping in to a new implementation of tracking errata applications, was the old way tested to ensure it was properly reading errata applications from remote execution jobs? I want to make sure the user-reported issue really was because jobs were cleaned up, and not because there's a legitimate bug around reading the applicability jobs (assuming they are not cleaned up). Edit: we chatted, and the feature did work when the jobs existed. |
ianballou
left a comment
ianballou
left a comment
There was a problem hiding this comment.
Took a first glance. The performance aspect of this is something I'd like to know more about. Is this putting significantly more load on postgres than anything else if a user applies multiple errata to, say, 1,000 hosts at once?
pavanshekar
force-pushed
the
issue-39185
branch
2 times, most recently
from
2931287 to
e44a1fd
Compare
ianballou
left a comment
ianballou
left a comment
There was a problem hiding this comment.
I'm thinking about the rebuild scenario. For a host, if it's provisioned via Foreman, users can rebuild them, which means the OS is likely completely reimaged.
Question then - today, if a host is rebuilt, is the applied errata report for that host the same after rebuild? Or is there any cleanup? Does this PR match that experience?
We may want to consider clearing the history data on rebuild since applied errata no longer apply to the host when it is reimaged. I can't think why someone would care about errata applications once a machine has abandoned its previous OS. This might be worth a question to the community.
I realized too that this report is a bit broken for image mode machines, but I don't think we have to worry too much about it since users shouldn't really be applying errata to those immutable machines.
You're right, when a host is rebuilt, the errata application history is no longer relevant since it applied to the old OS. But in this PR the old data won't be cleared. It will be cleaned up only when the host is deleted entirely (due to |
ianballou
left a comment
ianballou
left a comment
There was a problem hiding this comment.
Some feedback collected outside this PR:
- When a machine is rebuilt, the errata application history should be destroyed. Users are likely not interested in seeing errata applications from before a host was rebuilt since that data is gone. If they really need this info, they can go through the old task history, assuming it isn't cleaned up. There likely are build hooks Katello can use for this.
- With this history existing, we can now improve the applied errata report to tell if an applied erratum is still applicable to a host. This would be an indicator to admins that they need to go inspect why the application didn't result in the patch state that they expected. This would need to be addressed via the report template in the Foreman repo. This can be done now or filed as a future RFE.
| return [] unless proxy_action | ||
|
|
||
| script = proxy_action.input['script'] || '' | ||
| found = script.lines.find { |line| line.start_with?('# RESOLVED_ERRATA_IDS=') } || '' |
There was a problem hiding this comment.
Because of issues like https://projects.theforeman.org/issues/38384, these resolved IDs may not always be 100% accurate. I was hoping we could stop using that magic comment as part of this refactor somehow, but didn't really have the details of a solution. I think it's worth calling out here, though.
There was a problem hiding this comment.
Thanks for flagging this, Jeremy. I looked into a few alternatives: re-running the advisory query after the job completes risks capturing changed host state, storing resolved IDs as task input doesn't fix the root issue since we're still calling advisory_ids(), and parsing yum/dnf output would require handling OS-specific formats.
The magic comment is only used for install-by-search, the other extraction methods are solid. Do you think it's worth addressing this in this PR, or should we file a follow-up to tackle it alongside https://projects.theforeman.org/issues/38384 ?
There was a problem hiding this comment.
Yeah, definitely a follow-up, I just wanted to bring it to your attention. I wonder if there's a way we can get into the internals of the dnf command (since Katello proxies all those requests anyway) and directly figure out what errata are being applied that way, when the content is actually requested. Just a thought.
There was a problem hiding this comment.
The magic comment is only used for install-by-search, the other extraction methods are solid.
Kind of unfortunate since everything in the new UI uses the install-by-search templates. It's still like 98% accurate though, so not too bad.
There was a problem hiding this comment.
The dnf internals idea is interesting. That could give us ground truth.
pavanshekar
force-pushed
the
issue-39185
branch
2 times, most recently
from
9dbbf57 to
c04b6e9
Compare
There was a problem hiding this comment.
Tested again
- rake task runs and generates with correct string errata IDs
- applying an errata via REX correctly creates new ErrataApplications
- Deleting the RunHostsJob tasks and re-running the report correctly shows the errata applications, even when the tasks are missing
Just a couple more questions.
|
@aidenfine I don't really have any more comments here, but I think it would be a good idea to re-run your performance test since things changed a bit. |
pavanshekar
force-pushed
the
issue-39185
branch
2 times, most recently
from
973915e to
c7cd8f7
Compare
| applied_at: applied_at, | ||
| status: status | ||
| ) | ||
| [application] |
There was a problem hiding this comment.
Why does this return an array if it's only ever 1 or 0 items returned?
There was a problem hiding this comment.
I changed record_from_task to return the object directly or nil instead of an array.
| # Extract errata IDs from task | ||
| # @param task [ForemanTasks::Task] The task to extract from | ||
| # @param input_values_map [Hash] Optional pre-loaded map of template_invocation_id => errata_value | ||
| def self.extract_errata_ids_from_task(task, input_values_map = nil) |
There was a problem hiding this comment.
| def self.extract_errata_ids_from_task(task, input_values_map = nil) | |
| def self.extract_errata_ids_from_task(task) |
input_values_map is not used anymore.
There was a problem hiding this comment.
Removed the unused input_values_map parameter from extract_errata_ids_from_task and extract_from_template_input since bulk loading happens in the rake script now.
| return [] unless proxy_action | ||
|
|
||
| script = proxy_action.input['script'] || '' | ||
| found = script.lines.find { |line| line.start_with?('# RESOLVED_ERRATA_IDS=') } || '' |
There was a problem hiding this comment.
If there isn't one already, is it possible to add a unit test that ensures the magic errata IDs comment is there in the job template somehow?
There was a problem hiding this comment.
Added unit tests to verify the magic comment pattern exists in the search-based job template.
|
|
||
| # Helper method to bulk-load template invocation input values | ||
| # Only loads legacy template input ('errata'). Modern templates require Dynflow. | ||
| bulk_load_template_input_values = lambda do |tasks| |
There was a problem hiding this comment.
I find the use of lamdas here a bit difficult to read, could these just be methods? I don't see the lambdas closing over any variables.
There was a problem hiding this comment.
I converted all lambdas to methods as suggested. Changed from lambda do |params| to def self.method_name(params) and updated all calls from .call() to direct method invocation.
|
I don't think I have more comments than the above, and I'll rely on Jeremy's functional testing. +1 to having one more run of the perf test due to the recent changes. |
|
I added bulk SQL extraction to query the |
|
Latest performance test run: |
|
I don't think I have more comments than the above, and I'll rely on Jeremy's functional testing. +1 to having one more run of the perf test due to the recent changes.
Nice, for tens of thousands of hosts each with a year's worth of patching jobs, 17 minutes is fine. |
|
foreman_rh_cloud failures are unrelated. |
|
I added scoped_search to the ErrataApplication model and updated the report template helper to use scoped_search for date filtering instead of |
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
Re-tested and still works as expected
ianballou
left a comment
ianballou
left a comment
There was a problem hiding this comment.
Just one bit of unused code, otherwise +1. We can merge once that's taken care of.
| template_content = File.read(template_path) | ||
|
|
||
| # Verify the magic comment pattern is present in the template | ||
| assert_match(/# RESOLVED_ERRATA_IDS=/, template_content, |
There was a problem hiding this comment.
Normally I wouldn't want to have the test read that the code is there or not, but in this case, I think it's a helpful gate.
|
Fixed the migration counter to accurately report created vs skipped records by counting before/after all parallel threads complete, eliminating the race condition. First run: Second run: |
| @@ -120,6 +120,12 @@ def self.register_scheduled_task(task_class, cronline) | |||
| world.middleware.use ::Actions::Middleware::KeepLocale | |||
| end | |||
| end | |||
|
|
|||
| # Register middleware to track errata applications | |||
| ForemanTasks.dynflow.config.on_init do |world| | |||
There was a problem hiding this comment.
On my test Satellite, new errata application records were not being created. AI analysis came up with the following:
"""
ForemanTasks.dynflow.config.on_init do |world| defaults to on_init(true), which pushes the callback to @on_executor_init. These callbacks only fire when config.remote? is false (line 43 of dynflow/rails.rb). But in Satellite's dynflow-sidekiq architecture, the worker processes that actually execute REX/errata jobs set config.remote = true, so the executor-only callbacks never fire there. Only the orchestrator (which doesn't execute jobs) gets them.
"""
| ForemanTasks.dynflow.config.on_init do |world| | |
| ForemanTasks.dynflow.config.on_init(false) do |world| |
I do not see records when applying errata via "Install errata by search query" without this change.
There was a problem hiding this comment.
Implemented on_init(false) as suggested. Verified that errata application records are now being created at runtime.
pr-processor
Bot
added
Needs re-review
and removed
Waiting on contributor
Needs re-review
labels
4 participants
What are the changes introduced in this pull request?
Fixes the "Host - Applied Errata" report which was generating empty CSV files. The root cause was that the report relied on parsing Foreman task history every time it was generated, which was slow, brittle, and was not reliable. The fix introduces a dedicated database table called
katello_errata_applicationsthat persistently tracks errata applications as they happen. A Dynflow middleware component now automatically intercepts errata installation jobs when they complete and extracts relevant details to create database records. The report template has been updated to query this database table directly instead of parsing tasks, making report generation fast and reliable. The implementation includes a hybrid approach that falls back to the legacy task-parsing method if the table doesn't exist yet, ensuring backward compatibility during migration periods.Considerations taken when implementing this change?
The implementation follows the existing ContentViewHistory pattern to maintain consistency with Katello's established conventions for tracking historical events. The middleware was designed to run in the finalize phase of Dynflow actions to capture the final task state, with robust error handling to ensure recording failures don't break errata installation tasks. The solution handles two different job types: direct installations where errata IDs are specified upfront, and search query installations where IDs are resolved at runtime and extracted from the generated script. The database schema includes proper foreign keys, indexes on frequently queried columns, and a uniqueness constraint to prevent duplicate records. Comprehensive test coverage was added for both the model logic and report generation functionality to prevent future regressions. The test suite was developed completely using Claude Code to ensure thorough coverage of validations, scopes, edge cases, and integration scenarios.
What are the testing steps for this pull request?
Summary by Sourcery
Track applied errata in a dedicated database model and use it as the primary data source for errata reporting while keeping the legacy task-parsing path as a fallback.
New Features:
Bug Fixes:
Enhancements:
Deployment:
Tests:
Summary by CodeRabbit
New Features
Tests