HYPERFLEET-1152 - feat: separate resource_conditions table for generic resources#263
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughSummary by CodeRabbit
WalkthroughA new Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 545 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/api pkg/dao pkg/db/migrations | +1 |
Computed by hyperfleet-risk-scorer
![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)
pkg/db/migrations/202606290001_add_resource_conditions.go (1)
28-31: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDrop the standalone
resource_idindex unless you have a non-PK lookup to justify it.The primary key on
(resource_id, type)already covers the currentWHERE resource_id = ?fetch/delete path, so this adds another index to maintain on every full condition replace. That is extra write amplification on the hot path forUpdateConditions.Suggested change
- if err := tx.Exec( - "CREATE INDEX IF NOT EXISTS idx_resource_conditions_resource_id " + - "ON resource_conditions (resource_id);", - ).Error; err != nil { - return err - } - return nilAs per path instructions, apply the HyperFleet performance checks to new persistence paths.
🤖 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 `@pkg/db/migrations/202606290001_add_resource_conditions.go` around lines 28 - 31, The migration in add_resource_conditions.go is creating a standalone idx_resource_conditions_resource_id index that appears redundant with the primary key on (resource_id, type) for the current resource_id lookup path. Remove that index creation from the migration unless a separate non-PK query in UpdateConditions or related resource_conditions access truly needs it, and keep the migration focused on the table’s primary-key-backed access pattern.Source: Path instructions
🤖 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 `@pkg/dao/resource_condition.go`:
- Around line 33-35: The initial `Find(&existing)` error path in the
`resource_condition` DAO currently returns without marking the request
transaction for rollback, which can leave earlier writes committed. Update the
write flow that loads existing conditions to call `db.MarkForRollback(ctx, err)`
before returning the `Find` error, following the same pattern used for other
write failures in `pkg/dao`. Keep the fix localized to the resource condition
replacement path so any failure in this method prevents a partial commit.
In `@pkg/db/migrations/202606290001_add_resource_conditions.go`:
- Around line 12-24: The `UpdateConditions` migration currently uses
`resource_conditions` with `CREATE TABLE IF NOT EXISTS`, which can silently
accept an existing table with the wrong PK/FK/defaults and mask schema drift.
Replace this in the migration with a schema validation or fail-fast check in the
migration logic so `gormigrate` does not mark it applied unless
`resource_conditions` matches the expected definition; use the
`UpdateConditions` migration block and the `resource_conditions` table
definition to enforce exact structure.
---
Nitpick comments:
In `@pkg/db/migrations/202606290001_add_resource_conditions.go`:
- Around line 28-31: The migration in add_resource_conditions.go is creating a
standalone idx_resource_conditions_resource_id index that appears redundant with
the primary key on (resource_id, type) for the current resource_id lookup path.
Remove that index creation from the migration unless a separate non-PK query in
UpdateConditions or related resource_conditions access truly needs it, and keep
the migration focused on the table’s primary-key-backed access pattern.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 801f017f-ae5c-46c0-9d31-3a086fc0e835
📒 Files selected for processing (9)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/status_types.gopkg/dao/resource.gopkg/dao/resource_condition.gopkg/db/migrations/202606290001_add_resource_conditions.gopkg/db/migrations/migration_structs.gotest/integration/resource_conditions_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
| var existing []api.ResourceCondition | ||
| if err := g2.Where("resource_id = ?", resourceID).Find(&existing).Error; err != nil { | ||
| return err |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mark the initial query failure for rollback.
Line 34 returns the Find(&existing) error without db.MarkForRollback(ctx, err). In this write DAO, that can commit earlier mutations in the same request transaction while condition replacement fails, leaving the resource row and resource_conditions out of sync.
As per path instructions, pkg/dao/**: "On any write error, mark the transaction for rollback" and pkg/db/**: "Without this, the middleware will commit a partially-failed transaction."
Proposed fix
var existing []api.ResourceCondition
if err := g2.Where("resource_id = ?", resourceID).Find(&existing).Error; err != nil {
+ db.MarkForRollback(ctx, err)
return err
}📝 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.
| var existing []api.ResourceCondition | |
| if err := g2.Where("resource_id = ?", resourceID).Find(&existing).Error; err != nil { | |
| return err | |
| var existing []api.ResourceCondition | |
| if err := g2.Where("resource_id = ?", resourceID).Find(&existing).Error; err != nil { | |
| db.MarkForRollback(ctx, err) | |
| return err |
🤖 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 `@pkg/dao/resource_condition.go` around lines 33 - 35, The initial
`Find(&existing)` error path in the `resource_condition` DAO currently returns
without marking the request transaction for rollback, which can leave earlier
writes committed. Update the write flow that loads existing conditions to call
`db.MarkForRollback(ctx, err)` before returning the `Find` error, following the
same pattern used for other write failures in `pkg/dao`. Keep the fix localized
to the resource condition replacement path so any failure in this method
prevents a partial commit.
Source: Path instructions
| if err := tx.Exec(`CREATE TABLE IF NOT EXISTS resource_conditions ( | ||
| resource_id VARCHAR(255) NOT NULL, | ||
| type VARCHAR(100) NOT NULL, | ||
| status VARCHAR(10) NOT NULL, | ||
| reason TEXT, | ||
| message TEXT, | ||
| observed_generation INTEGER NOT NULL DEFAULT 0, | ||
| created_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| last_updated_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| last_transition_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| PRIMARY KEY (resource_id, type), | ||
| FOREIGN KEY (resource_id) REFERENCES resources(id) ON DELETE CASCADE | ||
| );`).Error; err != nil { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail on schema drift instead of masking it.
UpdateConditions depends on this table having the exact PK/FK/defaults described in the PR. CREATE TABLE IF NOT EXISTS will still return success if a partial/manual resource_conditions table already exists, and gormigrate will then record this migration as applied against the wrong schema.
Suggested change
- if err := tx.Exec(`CREATE TABLE IF NOT EXISTS resource_conditions (
+ if err := tx.Exec(`CREATE TABLE resource_conditions (
resource_id VARCHAR(255) NOT NULL,
type VARCHAR(100) NOT NULL,
status VARCHAR(10) NOT NULL,
reason TEXT,
message TEXT,
observed_generation INTEGER NOT NULL DEFAULT 0,
created_time TIMESTAMPTZ NOT NULL DEFAULT NOW(),
last_updated_time TIMESTAMPTZ NOT NULL DEFAULT NOW(),
last_transition_time TIMESTAMPTZ NOT NULL DEFAULT NOW(),
PRIMARY KEY (resource_id, type),
FOREIGN KEY (resource_id) REFERENCES resources(id) ON DELETE CASCADE
);`).Error; err != nil {
return err
}As per path instructions, migrations must be reviewed for backward compatibility and incompatible-schema risks.
📝 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.
| if err := tx.Exec(`CREATE TABLE IF NOT EXISTS resource_conditions ( | |
| resource_id VARCHAR(255) NOT NULL, | |
| type VARCHAR(100) NOT NULL, | |
| status VARCHAR(10) NOT NULL, | |
| reason TEXT, | |
| message TEXT, | |
| observed_generation INTEGER NOT NULL DEFAULT 0, | |
| created_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), | |
| last_updated_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), | |
| last_transition_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), | |
| PRIMARY KEY (resource_id, type), | |
| FOREIGN KEY (resource_id) REFERENCES resources(id) ON DELETE CASCADE | |
| );`).Error; err != nil { | |
| if err := tx.Exec(`CREATE TABLE resource_conditions ( | |
| resource_id VARCHAR(255) NOT NULL, | |
| type VARCHAR(100) NOT NULL, | |
| status VARCHAR(10) NOT NULL, | |
| reason TEXT, | |
| message TEXT, | |
| observed_generation INTEGER NOT NULL DEFAULT 0, | |
| created_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), | |
| last_updated_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), | |
| last_transition_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), | |
| PRIMARY KEY (resource_id, type), | |
| FOREIGN KEY (resource_id) REFERENCES resources(id) ON DELETE CASCADE | |
| );`).Error; err != nil { | |
| return err | |
| } |
🤖 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 `@pkg/db/migrations/202606290001_add_resource_conditions.go` around lines 12 -
24, The `UpdateConditions` migration currently uses `resource_conditions` with
`CREATE TABLE IF NOT EXISTS`, which can silently accept an existing table with
the wrong PK/FK/defaults and mask schema drift. Replace this in the migration
with a schema validation or fail-fast check in the migration logic so
`gormigrate` does not mark it applied unless `resource_conditions` matches the
expected definition; use the `UpdateConditions` migration block and the
`resource_conditions` table definition to enforce exact structure.
Source: Path instructions
1 participant
Summary
resource_conditionstable with composite PK(resource_id, type)and FK toresources(id)ON DELETE CASCADEResourceConditionDaointerface withUpdateConditionsthat atomically replaces all condition rows, preservingLastTransitionTimewhen status is unchanged andCreatedTimeacross updatesResourceDao.GetandGetByOwnernow preload conditions via GORM HasMany associationstatus.conditionsresponse fieldSaveoperations do not touch condition rows (Omit(clause.Associations)already in place)Architecture
UpdateConditionsdesigned to be called within adapter status upsert transactionDesign choices
ResourceConditionDao(not added toResourceDao) — followsAdapterStatusDaoprecedent for derived stateResourceConditionstruct — GORM table model + JSONB-deserializable type;ResourceIDfield usesjson:"-"for backward compatautoCreateTime) — codebase convention; delete+re-insert pattern requires pre-set value preservationTest coverage
TestResourceConditions_UpdateAndPreload(implicit via migration run)UpdateConditionsreplaces atomicallyTestResourceConditions_AtomicReplaceGetpreloads ConditionsTestResourceConditions_UpdateAndPreloadGetByOwnerpreloads ConditionsTestResourceConditions_GetByOwnerPreloadLastTransitionTimepreservedTestResourceConditions_LastTransitionTimePreservedCreatedTimepreservedTestResourceConditions_CreatedTimePreservedTestPresentResource_WithConditions,TestPresentResource_WithEmptyConditionsSavedoesn't touch conditionsTestResourceConditions_SaveDoesNotTouchConditionsTestResourceConditions_ClearWithEmptySliceTest plan
make verify-all(build + lint + 1272 unit tests)make test-integration(requires Docker — CI will run)