OSAC-1471, OSAC-4341: rename NetworkAttachment to ComputeNetworkAttachment - #479
OSAC-1471, OSAC-4341: rename NetworkAttachment to ComputeNetworkAttachment#479ori-amizur wants to merge 2 commits into
Conversation
|
@ori-amizur: This pull request references OSAC-1471 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 task to target the "5.1.0" version, but no target version was set. This pull request references OSAC-4341 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 task to target the "5.1.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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (12)
📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe PR renames ChangesCompute network attachment rename
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR renames network-attachment types while preserving wire fields, JSON tags, and generated artifacts, so no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
🤖 Finished Review · ✅ Success · Started 1:35 PM UTC · Completed 1:56 PM UTC Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: PR is a pure type-rename refactor across fulfillment-service, osac-operator, and osac-metering |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
requires-manual-review
|
🤖 Review · Commit: |
…hment Renames the ComputeInstanceSpec.network_attachments (field 14) message type from the generic NetworkAttachment to ComputeNetworkAttachment in both private and public fulfillment-service protos, matching the resource-prefixed naming already used by ClusterNetworkAttachment and BareMetalNetworkAttachment. Field number and field name are unchanged, so this is wire/JSON compatible with no database migration required. Also renames osac-operator's own hand-written ComputeInstance CRD Go type of the same name (companion ticket OSAC-4341), matching the operator's already-resource-prefixed ClusterNetworkAttachment CRD type. JSON tags are unchanged, so this is a Go/CRD-schema-internal rename only. Updates every production and test reference to the old type names in fulfillment-service and osac-operator, regenerates the vendored proto clients in osac-operator and osac-metering that are built from fulfillment-service's private proto, and regenerates osac-operator's CRD manifests and Helm chart via make manifests generate / make helm-crds. Also regenerates the rest of osac-operator's vendored proto client (baremetal_instance_type, cluster_template_type, cluster_type, instance_type_type, secret_type, and their protoopaque variants), which had drifted stale relative to fulfillment-service's current proto for reasons unrelated to this change. Touching any osac-operator file makes CI's "Check generated code" job re-verify the entire vendored tree, not just the files this rename cares about, so the pre-existing drift had to be resolved here too. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ori Amizur <oamizur@redhat.com>
|
🤖 Review · Commit: |
Two prose/comment references to the old NetworkAttachment name were missed by the initial rename (caught by automated PR review): docs/API.md's local-reference example, and a test-only proto message's doc comment (regenerated into the corresponding Go files). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Ori Amizur <oamizur@redhat.com>
|
🤖 Review · ❌ Terminated · Started 3:06 PM UTC · Ended 3:21 PM UTC Commit: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, ori-amizur 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 |
There was a problem hiding this comment.
See the review comment for full details.
Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:
fulfillment-service/proto/public/osac/public/v1/compute_instance_type.proto:45: [medium] breaking-api
The public proto renames NetworkAttachment to ComputeNetworkAttachment. While wire-compatible (field numbers and field names preserved), external repositories importing generated Go types (notably osac-test-infra) will see compile-time breakage when they next regenerate or vendor their proto clients. osac-ui (React/TypeScript) likely consumes REST/JSON using field names (network_attachments) which are unchanged and is probably unaffected.
Suggested fix: Coordinate with osac-test-infra maintainers to land a corresponding PR updating references from NetworkAttachment to ComputeNetworkAttachment before or alongside this merge.
osac-operator/api/v1alpha1/computeinstance_types_test.go(file-level): Line 271 · [low] stale-reference
The Describe block title still reads "NetworkAttachment immutability validation" after the type was renamed to ComputeNetworkAttachment. String-only issue with no compilation impact, but will confuse anyone searching for test coverage of the renamed type.
Suggested fix: Rename the string to "ComputeNetworkAttachment immutability validation".
osac-operator/internal/controller/computeinstance_validation_test.go(file-level): Line 93 · [low] stale-reference
The Describe block title still reads "NetworkAttachment immutability" after the type was renamed to ComputeNetworkAttachment. Same discoverability issue.
Suggested fix: Rename the string to "ComputeNetworkAttachment immutability".
|
🤖 Finished Review · ✅ Success · Started 3:06 PM UTC · Completed 3:21 PM UTC Commit: |
3 participants
Renames the ComputeInstanceSpec.network_attachments (field 14) message type from the generic NetworkAttachment to ComputeNetworkAttachment in both private and public fulfillment-service protos, matching the resource-prefixed naming already used by ClusterNetworkAttachment and BareMetalNetworkAttachment. Field number and field name are unchanged, so this is wire/JSON compatible with no database migration required.
Also renames osac-operator's own hand-written ComputeInstance CRD Go type of the same name (companion ticket OSAC-4341), matching the operator's already-resource-prefixed ClusterNetworkAttachment CRD type. JSON tags are unchanged, so this is a Go/CRD-schema-internal rename only.
Updates every production and test reference to the old type names in fulfillment-service and osac-operator, regenerates the vendored proto clients in osac-operator and osac-metering that are built from fulfillment-service's private proto, and regenerates osac-operator's CRD manifests and Helm chart via make manifests generate / make helm-crds.
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
Refactor
Documentation