Fixes #39222 - Remove TaskStatus model#11706
Conversation
📝 WalkthroughWalkthroughThis PR removes the Katello::TaskStatus model entirely, replacing it with Foreman's task system. Model associations are cleaned up, helper methods depending on task status are deleted, provider validation is tightened, API views migrate to Foreman templates, and the database schema is updated via migration. ChangesTaskStatus Model and Integration Removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
6a51737 to
36862c2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
db/migrate/20260220070318_drop_katello_task_statuses.katello.rb (1)
2-5: ⚖️ Poor tradeoffMake migration reversible.
The migration is currently irreversible because
remove_columndoesn't specify the column type/constraints anddrop_tabledoesn't preserve the table structure. While rollback may be unlikely for dead-code removal, reversible migrations are a Rails best practice for safety.♻️ Suggested refactor for reversibility
def change - remove_column :katello_providers, :task_status_id - drop_table :katello_task_statuses + remove_column :katello_providers, :task_status_id, :integer + drop_table :katello_task_statuses do |t| + t.string :uuid + t.integer :user_id + t.string :organization_id + t.string :state + t.datetime :start_time + t.datetime :finish_time + t.text :progress + t.text :parameters + t.text :result + t.timestamps + end endNote: Verify actual column types from
db/schema.rbbefore applying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@db/migrate/20260220070318_drop_katello_task_statuses.katello.rb` around lines 2 - 5, The migration is irreversible because remove_column(:katello_providers, :task_status_id) omits the column type/options and drop_table(:katello_task_statuses) does not record the table schema; make it reversible by rewriting change to use reversible do |dir| (or explicit up/down) that on dir.up removes the column and drops the table, and on dir.down re-creates katello_task_statuses with the original columns/indexes and re-adds task_status_id to katello_providers using the correct type and options (verify those types/constraints in db/schema.rb) so rollback can restore the exact prior schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@db/migrate/20260220070318_drop_katello_task_statuses.katello.rb`:
- Around line 2-5: The migration is irreversible because
remove_column(:katello_providers, :task_status_id) omits the column type/options
and drop_table(:katello_task_statuses) does not record the table schema; make it
reversible by rewriting change to use reversible do |dir| (or explicit up/down)
that on dir.up removes the column and drops the table, and on dir.down
re-creates katello_task_statuses with the original columns/indexes and re-adds
task_status_id to katello_providers using the correct type and options (verify
those types/constraints in db/schema.rb) so rollback can restore the exact prior
schema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54d0a597-4683-4fd3-8fd3-c856624ff76e
📒 Files selected for processing (19)
app/lib/katello/util/task_status.rbapp/models/katello/concerns/organization_extensions.rbapp/models/katello/concerns/user_extensions.rbapp/models/katello/content_view_version.rbapp/models/katello/provider.rbapp/models/katello/task_status.rbapp/views/katello/api/v2/common/async.json.rablapp/views/katello/api/v2/common/bulk_async.json.rablapp/views/katello/api/v2/host_errata/system_task.json.rablapp/views/katello/api/v2/providers/_provider.json.rablapp/views/katello/api/v2/tasks/index.json.rablapp/views/katello/api/v2/tasks/show.json.rablapp/views/katello/api/v2/tasks/task_status_show.json.rabldb/migrate/20260220070318_drop_katello_task_statuses.katello.rbspec/models/task_status_spec.rbtest/factories/task_status_factory.rbtest/fixtures/models/katello_task_statuses.ymltest/models/association_test.rbtest/support/fixtures_support.rb
💤 Files with no reviewable changes (14)
- app/views/katello/api/v2/tasks/task_status_show.json.rabl
- app/views/katello/api/v2/tasks/index.json.rabl
- test/fixtures/models/katello_task_statuses.yml
- test/factories/task_status_factory.rb
- app/models/katello/task_status.rb
- app/models/katello/concerns/user_extensions.rb
- app/views/katello/api/v2/tasks/show.json.rabl
- app/lib/katello/util/task_status.rb
- test/support/fixtures_support.rb
- app/models/katello/content_view_version.rb
- app/views/katello/api/v2/host_errata/system_task.json.rabl
- spec/models/task_status_spec.rb
- test/models/association_test.rb
- app/models/katello/concerns/organization_extensions.rb
What are the changes introduced in this pull request?
Considerations taken when implementing this change?
N/A
What are the testing steps for this pull request?
In both cases, verify the API output is unchanged.
Summary by CodeRabbit
Refactor
Chores