MGMT-23976: Add multi-tier storage types to Tenant CRD - #197
openshift-merge-bot[bot] merged 2 commits into
Conversation
|
@zszabo-rh: This pull request references MGMT-23976 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe Tenant API is refactored to replace a single Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/tenant_controller_test.go (1)
108-113: Assert the resolved tier as well as the name.
Tieris required by the new status type and is what the printer column displays. The helper should verify the controller writes the expected interim"default"tier, not just the StorageClass name.Proposed test assertion
} else { g.Expect(tenant.Status.StorageClasses).To(HaveLen(1)) g.Expect(tenant.Status.StorageClasses[0].Name).To(Equal(expectedSCName)) + g.Expect(tenant.Status.StorageClasses[0].Tier).To(Equal("default")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/tenant_controller_test.go` around lines 108 - 113, The test currently only asserts StorageClasses name but must also assert the resolved Tier; when expectedSCName is non-empty update the assertions in the test helper (where tenant.Status.StorageClasses and expectedSCName are used) to also check the StorageClasses[0].Tier equals the interim value ("default") so the controller wrote the required Tier field that the printer column relies on.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/tenant_types.go`:
- Around line 84-87: The StorageClasses field in tenant_types.go allows
duplicate tiers because the CRD lacks list-map annotations; update the
StorageClasses declaration (the field of type []ResolvedStorageClass) to include
kubebuilder markers to make the list a map keyed by the tier (e.g., add
kubebuilder:validation:ListType=map and
kubebuilder:validation:ListMapKeys={"tier"} immediately above the StorageClasses
field), then run make manifests to regenerate the CRD and confirm the generated
schema includes x-kubernetes-list-type: map and x-kubernetes-list-map-keys:
["tier"].
---
Nitpick comments:
In `@internal/controller/tenant_controller_test.go`:
- Around line 108-113: The test currently only asserts StorageClasses name but
must also assert the resolved Tier; when expectedSCName is non-empty update the
assertions in the test helper (where tenant.Status.StorageClasses and
expectedSCName are used) to also check the StorageClasses[0].Tier equals the
interim value ("default") so the controller wrote the required Tier field that
the printer column relies on.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9f8f39af-3985-47fb-8fe9-1499ae04a424
📒 Files selected for processing (6)
api/v1alpha1/tenant_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/osac.openshift.io_tenants.yamlinternal/controller/tenant_controller.gointernal/controller/tenant_controller_test.gointernal/controller/tenant_names.go
Add ResolvedStorageClass struct with name and tier fields to support multiple storage tiers per tenant. Replace the singular status.storageClass string with a status.storageClasses list. Add osac.openshift.io/storage-tier label constant for StorageClass tier identification. Update printer column to show resolved tier names. The controller is minimally adapted to write the existing single-tier result as a one-element list. Full multi-tier resolution logic will follow once the enhancement proposal (EP osac-project#32) is finalized. Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Generated-By: Claude Code (Anthropic)
8716b41 to
6867ffe
Compare
…dicate Add listType=map and listMapKey=tier to StorageClasses field so the CRD schema enforces one entry per tier and enables server-side apply merge semantics. Use osacStorageTierLabel in the StorageClass watch predicate so StorageClasses without the storage-tier label are filtered out before triggering reconciliation. Signed-off-by: Zoltan Szabo <zszabo@redhat.com> Generated-By: Claude Code (Anthropic)
| func storageClassTenantPredicate() predicate.Predicate { | ||
| return predicate.NewPredicateFuncs(func(obj client.Object) bool { | ||
| _, exists := obj.GetLabels()[osacTenantAnnotation] | ||
| return exists | ||
| labels := obj.GetLabels() | ||
| _, hasTenant := labels[osacTenantAnnotation] | ||
| _, hasTier := labels[osacStorageTierLabel] | ||
| return hasTenant && hasTier | ||
| }) | ||
| } |
There was a problem hiding this comment.
The predicate requires both labels, but getTenantStorageClass (line 206) still queries by tenant label only. This is intentionally ahead of the resolution logic until the full multi-tier work lands (EP #32). Consider adding a brief code comment noting that StorageClasses without the storage-tier label won't trigger reconciliation, so developers working on this area understand the mismatch is intentional while multi-tier resolution is in progress.
akshaynadkarni
left a comment
akshaynadkarni
left a comment
There was a problem hiding this comment.
Left a minor comment. You can address in a follow-up PR in case it will be a while until the watch and reconciliation logic matches.
LGTM
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni, zszabo-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Summary
ResolvedStorageClassstruct withnameandtierfields to support multiple storage tiers per tenant (EP #32)status.storageClass(string) withstatus.storageClasses(list ofResolvedStorageClass)osac.openshift.io/storage-tierlabel constantNotes
The controller is minimally adapted to write the existing single-tier result as a one-element list with
tier: "default". Full multi-tier resolution logic will follow in a separate PR once EP #32 is finalized.Test plan
make testpasses — existing tenant controller tests adapted for the new list formatmake manifestsregenerates CRD YAML with correct schemago build ./...compiles cleanlyJira: MGMT-23976
Summary by CodeRabbit