feat(db): create systems table#2754
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:
WalkthroughAdds a Rails migration that creates a ChangesSystems Table Creation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
db/migrate/20260601090600_create_systems.rb (1)
3-19: Consider adding indexes on frequently queried columns.The migration doesn't include indexes on
org_id,account, ordeleted_at, which are likely to be used in WHERE clauses for common queries:
org_id: Filtering systems by organizationdeleted_at: Soft-delete queries (e.g.,WHERE deleted_at IS NULL)- Composite index on
(org_id, deleted_at): For queries filtering by bothThese indexes can be added later once query patterns are established and performance monitoring indicates they're beneficial. Consider adding them in a follow-up migration if query performance becomes a concern.
Example indexes for common query patterns
add_index :systems, :org_id add_index :systems, :deleted_at # Or a composite index for queries filtering by both: add_index :systems, [:org_id, :deleted_at]🤖 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/20260601090600_create_systems.rb` around lines 3 - 19, The migration creates the systems table but omits indexes on frequently queried columns; add indexes for org_id and deleted_at (and account if used in queries) and consider a composite index on [:org_id, :deleted_at] to optimize common WHERE clauses; update the migration by adding add_index :systems, :org_id, add_index :systems, :deleted_at, optionally add_index :systems, :account, and/or add_index :systems, [:org_id, :deleted_at] (use the existing create_table :systems and add_index :systems, :insights_id context to place these).
🤖 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.
Inline comments:
In `@db/migrate/20260601090600_create_systems.rb`:
- Around line 8-9: The migration currently defines custom timestamp columns
t.datetime :created and t.datetime :updated alongside Rails' t.timestamps,
creating duplicate timestamp columns; remove the custom t.datetime :created and
t.datetime :updated lines and keep t.timestamps so ActiveRecord manages
created_at/updated_at automatically (locate these symbols in the CreateSystems
migration and delete the two custom timestamp declarations).
- Line 12: Update the migration definition for the groups JSONB column to
prevent NULLs: change the column declaration "t.jsonb :groups, default: []" to
include "null: false" (i.e. "t.jsonb :groups, default: [], null: false") in the
CreateSystems migration (the line with t.jsonb :groups) so the groups field has
the same non-null constraint as tags and system_profile; if this migration has
already run in environments, instead add a separate migration that sets existing
NULL groups to [] and then alters the column to set NOT NULL on the systems
table.
---
Nitpick comments:
In `@db/migrate/20260601090600_create_systems.rb`:
- Around line 3-19: The migration creates the systems table but omits indexes on
frequently queried columns; add indexes for org_id and deleted_at (and account
if used in queries) and consider a composite index on [:org_id, :deleted_at] to
optimize common WHERE clauses; update the migration by adding add_index
:systems, :org_id, add_index :systems, :deleted_at, optionally add_index
:systems, :account, and/or add_index :systems, [:org_id, :deleted_at] (use the
existing create_table :systems and add_index :systems, :insights_id context to
place these).
🪄 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: Enterprise
Run ID: a37639a0-748b-441e-b682-106245aee8e0
📒 Files selected for processing (1)
db/migrate/20260601090600_create_systems.rb
| t.datetime :created, null: false | ||
| t.datetime :stale_timestamp, null: false | ||
| t.jsonb :system_profile, null: false, default: {} | ||
| t.jsonb :groups, default: [] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add null: false constraint to groups for consistency.
The groups field allows NULL values, unlike the other JSONB fields (tags and system_profile) which have null: false constraints. This inconsistency could lead to unexpected behavior when querying or processing group data.
🛡️ Proposed fix to add null constraint
- t.jsonb :groups, default: []
+ t.jsonb :groups, null: false, default: []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.jsonb :groups, default: [] | |
| t.jsonb :groups, null: false, default: [] |
🤖 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/20260601090600_create_systems.rb` at line 12, Update the migration
definition for the groups JSONB column to prevent NULLs: change the column
declaration "t.jsonb :groups, default: []" to include "null: false" (i.e.
"t.jsonb :groups, default: [], null: false") in the CreateSystems migration (the
line with t.jsonb :groups) so the groups field has the same non-null constraint
as tags and system_profile; if this migration has already run in environments,
instead add a separate migration that sets existing NULL groups to [] and then
alters the column to set NOT NULL on the systems table.
petrblaho
force-pushed
the
ciao-cyndi-systems-table
branch
2 times, most recently
from
c3436ef to
8983412
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
db/schema.rb (1)
13-13:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate db/schema.rb schema version to latest migration timestamp
db/schema.rb sets
ActiveRecord::Schema[8.1].define(version: 2026_05_13_140100)but the newest migration isdb/migrate/20260601090600_create_systems.rb, so the schema version should be2026_06_01_090600.🔧 Proposed fix
-ActiveRecord::Schema[8.1].define(version: 2026_05_13_140100) do +ActiveRecord::Schema[8.1].define(version: 2026_06_01_090600) do🤖 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/schema.rb` at line 13, The schema version in db/schema.rb is outdated; update the value passed to ActiveRecord::Schema[...] .define(...) from 2026_05_13_140100 to the latest migration timestamp 2026_06_01_090600 so the schema version matches the newest migration (migration file 20260601090600_create_systems.rb); locate the line containing ActiveRecord::Schema[8.1].define(version: ...) and replace the version token accordingly.
🧹 Nitpick comments (2)
db/schema.rb (2)
237-237: ⚡ Quick winConsider adding
null: falseconstraint togroupsfor consistency.The
groupscolumn has a default value of[]but lacks anull: falseconstraint, unlike thetagsandsystem_profilecolumns which both havenull: false. This inconsistency meansgroupscould be NULL despite having a default, requiring extra null checks in application code.For consistency and to prevent NULL/empty-array confusion, consider adding
null: falseto thegroupscolumn definition.🔧 Suggested improvement
Update the migration file
db/migrate/20260601090600_create_systems.rb:- t.jsonb :groups, default: [] + t.jsonb :groups, default: [], null: falseThen regenerate the schema:
rails db:migrate:redo VERSION=20260601090600🤖 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/schema.rb` at line 237, The groups JSONB column currently has default: [] but lacks a null: false constraint; update the migration that creates the systems table (the CreateSystems migration) to change the column definition for "groups" to t.jsonb "groups", default: [], null: false so the DB enforces non-null values, then re-run the migration (redo the migration version that creates systems) and regenerate the schema so db/schema.rb reflects the new null: false constraint.
232-232: 💤 Low valueConsider whether
accountshould be required.The
accountcolumn is nullable whileorg_idis required. If account information is always available and should be tracked for all systems, addingnull: falsewould enforce data integrity.However, if systems can exist without account assignments (e.g., during migration scenarios or when ingesting from external sources that only provide
org_id), the current nullable definition is appropriate.🤖 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/schema.rb` at line 232, Decide whether the "account" column should be required: if it must always be present, add a migration to set any existing NULL "account" values to a valid default or backfill from related data, then alter the column to add NOT NULL (e.g., change_column_null :table_name, :account, false) and add a model-level validation (validates :account, presence: true); if it can remain optional, leave the schema as-is but consider adding a comment explaining why it's nullable. Ensure you reference the "account" and "org_id" columns when making the change so the migration and model validation target the correct column.
🤖 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.
Inline comments:
In `@db/schema.rb`:
- Around line 233-234: Decide and document a single source-of-truth for system
timestamps: either remove the custom columns (:created / :updated) from the
CreateSystems migration (db/migrate/20260601090600_create_systems.rb) and rely
on Rails timestamps (created_at/updated_at), or keep the custom columns and stop
using t.timestamps; then update the System model to explicitly map/alias the
authoritative columns (e.g., alias_attribute :created, :created_at or vice
versa) and ensure record_timestamps behavior is correct, update the API schema
mapping in spec/api/v2/schemas/system.rb so the API’s `updated` field points to
the chosen column, and add a short comment in the migration and the System model
explaining which pair is authoritative and whether they are kept in sync.
---
Outside diff comments:
In `@db/schema.rb`:
- Line 13: The schema version in db/schema.rb is outdated; update the value
passed to ActiveRecord::Schema[...] .define(...) from 2026_05_13_140100 to the
latest migration timestamp 2026_06_01_090600 so the schema version matches the
newest migration (migration file 20260601090600_create_systems.rb); locate the
line containing ActiveRecord::Schema[8.1].define(version: ...) and replace the
version token accordingly.
---
Nitpick comments:
In `@db/schema.rb`:
- Line 237: The groups JSONB column currently has default: [] but lacks a null:
false constraint; update the migration that creates the systems table (the
CreateSystems migration) to change the column definition for "groups" to t.jsonb
"groups", default: [], null: false so the DB enforces non-null values, then
re-run the migration (redo the migration version that creates systems) and
regenerate the schema so db/schema.rb reflects the new null: false constraint.
- Line 232: Decide whether the "account" column should be required: if it must
always be present, add a migration to set any existing NULL "account" values to
a valid default or backfill from related data, then alter the column to add NOT
NULL (e.g., change_column_null :table_name, :account, false) and add a
model-level validation (validates :account, presence: true); if it can remain
optional, leave the schema as-is but consider adding a comment explaining why
it's nullable. Ensure you reference the "account" and "org_id" columns when
making the change so the migration and model validation target the correct
column.
🪄 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: Enterprise
Run ID: 3d7d12c5-f2c5-4af0-bf01-9da4c3d9b7e1
📒 Files selected for processing (2)
db/migrate/20260601090600_create_systems.rbdb/schema.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- db/migrate/20260601090600_create_systems.rb
petrblaho
force-pushed
the
ciao-cyndi-systems-table
branch
2 times, most recently
from
3f6ae31 to
508eedc
Compare
|
/retest |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2754 +/- ##
=======================================
Coverage 93.53% 93.53%
=======================================
Files 184 184
Lines 4390 4390
=======================================
Hits 4106 4106
Misses 284 284 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Pretty please hold this back until we merge the DB remodel. |
petrblaho
force-pushed
the
ciao-cyndi-systems-table
branch
from
508eedc to
70e150e
Compare
|
Seemingly unrelated kafka consumer test failure. For the record, we should go forward with this. The DB remodel release will happen next week due to stage instability... |
petrblaho
force-pushed
the
ciao-cyndi-systems-table
branch
2 times, most recently
from
0687777 to
7daa784
Compare
Branch protection rule check failed
petrblaho
force-pushed
the
ciao-cyndi-systems-table
branch
2 times, most recently
from
6cb4b6e to
e612860
Compare
Setup UUID-based systems table with org_id, tags, system_profile, and unique insights_id to host data locally. https://redhat.atlassian.net/browse/RHINENG-23278
petrblaho
force-pushed
the
ciao-cyndi-systems-table
branch
from
e612860 to
dc33671
Compare
3 participants
Setup UUID-based systems table with org_id, tags, system_profile,
and unique insights_id to host data locally.
https://redhat.atlassian.net/browse/RHINENG-23278
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist