HYPERFLEET-1160 - feat: separate resource_labels table for generic resource labels#270
Open
kuudori wants to merge 1 commit into
Open
Conversation
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (15)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughSummary by CodeRabbit
WalkthroughResource labels now use a normalized Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceService
participant ResourceDAO
participant ResourceLabelDAO
participant Database
Client->>ResourceService: create or patch resource
ResourceService->>ResourceDAO: persist resource row
ResourceService->>ResourceLabelDAO: ReplaceLabels
ResourceLabelDAO->>Database: delete old labels
ResourceLabelDAO->>Database: insert label rows
ResourceService-->>Client: resource with Labels slice
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 2 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 369 lines (>200) | +1 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/api pkg/dao pkg/db pkg/db/migrations plugins/resources | +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
🤖 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/api/presenters/resource.go`:
- Around line 123-132: `convertLabelsToModel` currently forwards user-supplied
label keys and values without enforcing the `ResourceLabel` size limits, so
oversized input falls through as a DB constraint error instead of a validation
error. Add boundary validation in `convertLabelsToModel` for key and value
length (matching the `pkg/api/resource_label.go` limits) and return a typed
validation error before persistence. Update `ConvertResource` to propagate the
new error return, and apply the same validation flow in `applyResourcePatch` so
both create and patch paths reject invalid labels consistently.
In `@pkg/db/migrations/202607010001_add_resource_labels.go`:
- Around line 11-31: The migration in the resource labels `Migrate` function
drops `resources.labels` without preserving existing data, and it has no
rollback path. Before the `ALTER TABLE resources DROP COLUMN IF EXISTS labels`
step, backfill `resource_labels` from the existing `resources.labels` JSONB data
in the same migration, then remove the column only after the copy succeeds. Also
add a `Rollback` for this migration that recreates `resources.labels` and
restores data from `resource_labels` using the migration’s existing symbols
(`Migrate`, `Rollback`, `resource_labels`, `resources.labels`) so rollbacks and
rolling deploys remain safe.
🪄 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: 123fe713-ae4e-4454-b5cf-0d71a78eeefd
📒 Files selected for processing (15)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_label.gopkg/dao/resource.gopkg/dao/resource_label.gopkg/db/migrations/202607010001_add_resource_labels.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/services/resource.gopkg/services/resource_test.goplugins/resources/plugin.gotest/integration/channels_test.gotest/integration/resource_helpers.gotest/integration/versions_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)
kuudori
force-pushed
the
HYPERFLEET-1160-feat-resource-labels
branch
2 times, most recently
from
32471e8 to
9f97971
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/db/migrations/202607010001_add_resource_labels.go`:
- Around line 12-24: The migration in Migrate creates resource_labels and drops
resources.labels, but it never adds the planned (key, value) index. Update the
migration to create that composite index as part of the same transaction, using
the resource_labels table definition and keeping the existing Exec error
handling consistent. Make sure the new index creation is placed near the table
setup so the schema change fully matches the PR objective.
🪄 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: 956f56ad-d3c4-497f-a25b-608e88b90765
📒 Files selected for processing (15)
pkg/api/presenters/resource.gopkg/api/presenters/resource_test.gopkg/api/resource.gopkg/api/resource_label.gopkg/dao/resource.gopkg/dao/resource_label.gopkg/db/migrations/202607010001_add_resource_labels.gopkg/db/migrations/migration_structs.gopkg/db/sql_helpers.gopkg/services/resource.gopkg/services/resource_test.goplugins/resources/plugin.gotest/integration/channels_test.gotest/integration/resource_helpers.gotest/integration/versions_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)
✅ Files skipped from review due to trivial changes (3)
- pkg/db/migrations/migration_structs.go
- pkg/api/resource_label.go
- pkg/db/sql_helpers.go
🚧 Files skipped from review as they are similar to previous changes (11)
- plugins/resources/plugin.go
- pkg/api/presenters/resource_test.go
- pkg/api/resource.go
- pkg/services/resource_test.go
- pkg/dao/resource.go
- test/integration/versions_test.go
- test/integration/channels_test.go
- test/integration/resource_helpers.go
- pkg/api/presenters/resource.go
- pkg/services/resource.go
- pkg/dao/resource_label.go
kuudori
force-pushed
the
HYPERFLEET-1160-feat-resource-labels
branch
from
9f97971 to
6640b02
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
labels JSONBcolumn onresourcestable with dedicatedresource_labelstable (composite PK(resource_id, key), FK with CASCADE delete)ResourceLabelmodel,ResourceLabelDaowith delete+insertReplaceLabels, preload labels on all DAO read methodsReplaceLabelsexplicitly (DAO usesOmit(clause.Associations))labels.xxxTSL search for Resource entities until JOIN-based query support addedWhy
JSONB cannot enforce label key uniqueness at DB level, doesn't support efficient label-filter queries without GIN indexes, and conflates user-writable fields with system fields. Separate table enforces uniqueness via composite PK and enables indexed lookups. Per design doc §4.1-4.2.
Test plan
make test— 1314 tests)make test-integration— 237 tests, 3 skipped)golangci-lint— 0 issues)TestVersionList/ListByLabelskipped — TSL parser needs changes for JOIN-based label queries on separate tableTicket
https://redhat-internal.atlassian.net/browse/HYPERFLEET-1160