MGMT-23735: add PublicIP CRD type definitions - #188
openshift-merge-bot[bot] merged 6 commits into
Conversation
|
@akshaynadkarni: This pull request references MGMT-23735 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 story 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. |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akshaynadkarni 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new v1alpha1 Kubernetes CustomResourceDefinition "PublicIP" with API types, status enums, condition helpers, unit tests, autogenerated deepcopy methods, CRD manifest and kustomization entry. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
/hold |
|
@akshaynadkarni: This pull request references MGMT-23735 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 story 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. |
|
@akshaynadkarni: This pull request references MGMT-23735 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 story 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
config/crd/bases/osac.openshift.io_publicips.yaml (1)
19-35: Print columns LGTM; consider marking some aspriority: 1.By convention,
oc getshows onlypriority: 0columns by default and hidespriority: 1behind-o wide.Pool,Address,State,Phase,Ageis 5 columns which may feel noisy on narrow terminals. Optionally demotePool(and/orPhase, which is derivable fromState) topriority: 1. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crd/bases/osac.openshift.io_publicips.yaml` around lines 19 - 35, The additionalPrinterColumns block currently lists Pool (.spec.pool), Address (.status.address), State (.status.state), Phase (.status.phase) and Age (.metadata.creationTimestamp) without priorities; to reduce width by default, update the additionalPrinterColumns entries (e.g., the column for Pool and/or Phase) to include "priority: 1" so they are hidden from the default oc get view (leave Address, State or Age at default priority: 0 as desired); modify the entries in the additionalPrinterColumns array in the CRD (referencing the jsonPath values) to add the priority field for the chosen columns.api/v1alpha1/publicip_types.go (2)
142-145: RedundantGetName()method.
PublicIPembedsmetav1.ObjectMeta, which already provides aGetName() stringaccessor (satisfyingmetav1.Object). This override just returns the same value and adds no behavior, while shadowing the embedded method. Consider removing it unless you explicitly need the method on the value receiver pattern elsewhere.♻️ Proposed removal
-// GetName returns the name of the PublicIP resource -func (p *PublicIP) GetName() string { - return p.ObjectMeta.Name -} - func init() { SchemeBuilder.Register(&PublicIP{}, &PublicIPList{}) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/publicip_types.go` around lines 142 - 145, Remove the redundant GetName method on the PublicIP type: delete the func (p *PublicIP) GetName() string { return p.ObjectMeta.Name } declaration (it shadows the embedded metav1.ObjectMeta's GetName). Ensure no code relies on that specific value-receiver method signature—if callers expect a value receiver, update them to use the embedded GetName() or accept the pointer method on the embedded ObjectMeta. After removal, run the tests and a quick `go vet`/format to verify no references remain.
92-94: Strayprotobuftag on Conditions.The
protobuf:"bytes,1,rep,name=conditions"struct tag is meaningful for in-tree k8s API types generated via protoc, but this CRD type isn't protobuf-generated. It's harmless but misleading. Since PR description notes this mirrors thePublicIPPoolpattern, feel free to ignore — but worth cleaning up both at once if you agree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/publicip_types.go` around lines 92 - 94, The Conditions field currently includes a stray protobuf struct tag; remove the protobuf:"bytes,1,rep,name=conditions" portion from the tag on the Conditions field in api/v1alpha1/publicip_types.go (the Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"`), and apply the same cleanup to the matching Conditions tag on the PublicIPPool type if present so the CRD types only include JSON/patch tags.api/v1alpha1/publicip_types_test.go (1)
93-117: Consider strengthening condition helper tests.Current coverage verifies set→get round-trip and missing→nil. Two easy additions would catch real regressions:
- Assert
SetPublicIPStatusConditionwith the same type but different status transitionsLastTransitionTime(i.e.meta.SetStatusConditionsemantics are preserved).- Assert retrieved
Type,Reason,Messagematch what was set (not justStatus).Optional — not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/publicip_types_test.go` around lines 93 - 117, Add assertions to the PublicIP condition helper tests to ensure SetPublicIPStatusCondition preserves all condition fields and transition semantics: after calling SetPublicIPStatusCondition(ip, condition) in the "should set and get a condition" test, assert that the retrieved condition from GetPublicIPStatusCondition(ip, PublicIPConditionConfigurationApplied) has the same Type, Reason, Message and LastTransitionTime behavior (e.g., that LastTransitionTime updates when status changes matching meta.SetStatusCondition semantics) in addition to the existing Status check; also add a subtest that calls SetPublicIPStatusCondition twice with the same Type but a different Status/LastTransitionTime and assert LastTransitionTime changed and fields remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/v1alpha1/publicip_types_test.go`:
- Around line 93-117: Add assertions to the PublicIP condition helper tests to
ensure SetPublicIPStatusCondition preserves all condition fields and transition
semantics: after calling SetPublicIPStatusCondition(ip, condition) in the
"should set and get a condition" test, assert that the retrieved condition from
GetPublicIPStatusCondition(ip, PublicIPConditionConfigurationApplied) has the
same Type, Reason, Message and LastTransitionTime behavior (e.g., that
LastTransitionTime updates when status changes matching meta.SetStatusCondition
semantics) in addition to the existing Status check; also add a subtest that
calls SetPublicIPStatusCondition twice with the same Type but a different
Status/LastTransitionTime and assert LastTransitionTime changed and fields
remain consistent.
In `@api/v1alpha1/publicip_types.go`:
- Around line 142-145: Remove the redundant GetName method on the PublicIP type:
delete the func (p *PublicIP) GetName() string { return p.ObjectMeta.Name }
declaration (it shadows the embedded metav1.ObjectMeta's GetName). Ensure no
code relies on that specific value-receiver method signature—if callers expect a
value receiver, update them to use the embedded GetName() or accept the pointer
method on the embedded ObjectMeta. After removal, run the tests and a quick `go
vet`/format to verify no references remain.
- Around line 92-94: The Conditions field currently includes a stray protobuf
struct tag; remove the protobuf:"bytes,1,rep,name=conditions" portion from the
tag on the Conditions field in api/v1alpha1/publicip_types.go (the Conditions
[]metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge"
patchMergeKey:"type"`), and apply the same cleanup to the matching Conditions
tag on the PublicIPPool type if present so the CRD types only include JSON/patch
tags.
In `@config/crd/bases/osac.openshift.io_publicips.yaml`:
- Around line 19-35: The additionalPrinterColumns block currently lists Pool
(.spec.pool), Address (.status.address), State (.status.state), Phase
(.status.phase) and Age (.metadata.creationTimestamp) without priorities; to
reduce width by default, update the additionalPrinterColumns entries (e.g., the
column for Pool and/or Phase) to include "priority: 1" so they are hidden from
the default oc get view (leave Address, State or Age at default priority: 0 as
desired); modify the entries in the additionalPrinterColumns array in the CRD
(referencing the jsonPath values) to add the priority field for the chosen
columns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 879f8716-9e71-4650-bf7d-d90abc0915b8
📒 Files selected for processing (6)
api/v1alpha1/publicip_conditions.goapi/v1alpha1/publicip_types.goapi/v1alpha1/publicip_types_test.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/osac.openshift.io_publicips.yamlconfig/crd/kustomization.yaml
eranco74
left a comment
eranco74
left a comment
There was a problem hiding this comment.
Code Review
Clean PR — follows the PublicIPPool pattern consistently. One thing to fix before merge:
Needs rebase: The branch was forked before publicippools.yaml was added to config/crd/kustomization.yaml. The diff adds it as a new line, but it already exists on main. This will either conflict or produce a duplicate entry. Please rebase on latest main.
Minor nitpicks (non-blocking, matches existing PublicIPPool patterns):
GetName()onPublicIPshadows the embeddedmetav1.ObjectMeta.GetName()— redundant but consistent withPublicIPPool- Stray
protobufstruct tag onConditionsfield — meaningless for CRD types but again matchesPublicIPPool
Everything else looks good — CEL immutability, state/phase separation, shared JobStatus reuse, tests, and generated artifacts are all solid.
…er in kustomization
Ginkgo tests for PublicIP spec, phase types, state types, condition type, GetName method, and condition helpers (Set/Get). Follows existing PublicIPPool test patterns. Assisted-by: Claude
Prevents the API server from accepting explicit empty strings for the optional ComputeInstance attachment reference. Without this, computeInstance: "" is accepted as valid, forcing the controller to distinguish "not set" from "set to empty." Assisted-by: Cursor/Claude
Aligns with proto change ac8e3a7 in fulfillment-service which added PUBLIC_IP_STATE_FAILED. Without a failure state, a failed allocation or release would be stuck in Pending/Releasing with the error only in status.message. Assisted-by: Cursor/Claude
After rebasing on main, the publicippools.yaml entry that was added as an auto-fix now duplicates the one already on main. Remove the duplicate and append publicips.yaml after publicippools.yaml. Assisted-by: Cursor/Claude
ff00fc7 to
b19c528
Compare
@eranco74 I have rebased and resolved conflicts. PTAL. |
|
/lgtm |
|
/unhold |
3 participants
Summary
MGMT-23735: Define the PublicIP Custom Resource Definition in osac-operator.
Adds the complete PublicIP CRD type hierarchy:
pool(required, immutable via CELself == oldSelf, MinLength=1) andcomputeInstance(optional, MinLength=1)JobStatus), conditionsGetPublicIPStatusCondition/SetPublicIPStatusCondition(package-level functions, matching PublicIPPool pattern)oc get publicipsconfig/crd/kustomization.yamlTesting
Related PRs
Ticket
MGMT-23735
Assisted-by: Cursor/Claude
Summary by CodeRabbit
New Features
Tests