chore: Implementing layered proto conversion for VPC handlers#505
Open
chet wants to merge 1 commit intoNVIDIA:mainfrom
Open
chore: Implementing layered proto conversion for VPC handlers#505chet wants to merge 1 commit intoNVIDIA:mainfrom
chet wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Implements a layered convention for proto-conversion methods that the rest of the proto handling rollout will adopt. This was already done for `Expected*` components, but VPC had some additional bits that I was fiddling with (I did NVIDIA#501 but that was some stuff to be shared across `vpc`, `vpcprefix`, and `vpcpeering`). All said, this splits proto conversion into clear layers, and applies to `vpc` handlers: - A primary `ToProto`/`FromProto` on the DB entity. - Followed by `ToProto` on each API request type for request-shape conversion. - With entity-level methods for handlers with no API request body (e.g. delete). `AGENTS.md` has been further updated to document this layering. Tests added! Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Summary by CodeRabbit
WalkthroughThis PR refactors VPC proto conversion methods to follow a centralized, layered pattern: DB entities provide canonical entity ↔ proto conversion ( ChangesVPC Proto Conversion Centralization
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-05-07 23:19:11 UTC | Commit: d14c97a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@AGENTS.md`:
- Around line 189-197: The doc overstates FromProto's “leave receiver unchanged
on parse failure” guarantee; update the AGENTS.md text for the ToProto/FromProto
contract to explicitly state that FromProto treats a nil proto as a no-op and
generally does not modify existing fields on parse errors, but that optional
pointer fields and other explicitly-cleared fields may be reset when the proto
contains invalid optional values (e.g., the Vpc reference implementation clears
NVLinkLogicalPartitionID on an invalid UUID). Mention FromProto and
Vpc/NVLinkLogicalPartitionID by name so implementers know which fields may be
cleared vs preserved.
In `@api/pkg/api/model/vpc.go`:
- Around line 232-247: The current APIVpcUpdateRequest.ToProto always takes
NetworkSecurityGroupId from the merged DB vpc, which loses explicit clears when
asur.NetworkSecurityGroupID is set to an empty string; modify
APIVpcUpdateRequest.ToProto so it prefers the API request override when
asur.NetworkSecurityGroupID != nil (using its value, which may be ""), and falls
back to vpc.ToProto().NetworkSecurityGroupId only when
asur.NetworkSecurityGroupID is nil; update the returned
cwssaws.VpcUpdateRequest.NetworkSecurityGroupId accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 694f1f3c-6dc8-4e3d-9f52-5d54788f2f2f
📒 Files selected for processing (6)
AGENTS.mdapi/pkg/api/handler/vpc.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.godb/pkg/db/model/vpc.godb/pkg/db/model/vpc_test.go
Comment on lines
+189
to
+197
| 1. **Primary entity ↔ proto entity** lives on the DB model: | ||
| `func (m *T) ToProto(...) *protoT` and | ||
| `func (m *T) FromProto(p *protoT, ...)` — symmetric pair, defined | ||
| together. `FromProto` mutates the receiver, treats a `nil` proto as a | ||
| no-op, and returns no error (callers pre-validate anything risky like | ||
| UUID strings, and the method leaves the receiver field unchanged on | ||
| parse failure). Optional pointer fields are explicitly cleared when | ||
| the proto omits them, so `FromProto` is a clean reset rather than a | ||
| partial merge. |
Contributor
There was a problem hiding this comment.
Tighten the FromProto parse-failure contract.
This section says parse failures leave receiver fields unchanged, but the new Vpc reference implementation clears NVLinkLogicalPartitionID when the proto carries an invalid UUID. Please narrow this wording to the fields that truly preserve prior state, or document invalid optional values as clears so future implementations do not inherit the wrong contract.
🤖 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 `@AGENTS.md` around lines 189 - 197, The doc overstates FromProto's “leave
receiver unchanged on parse failure” guarantee; update the AGENTS.md text for
the ToProto/FromProto contract to explicitly state that FromProto treats a nil
proto as a no-op and generally does not modify existing fields on parse errors,
but that optional pointer fields and other explicitly-cleared fields may be
reset when the proto contains invalid optional values (e.g., the Vpc reference
implementation clears NVLinkLogicalPartitionID on an invalid UUID). Mention
FromProto and Vpc/NVLinkLogicalPartitionID by name so implementers know which
fields may be cleared vs preserved.
Comment on lines
+232
to
+247
| // ToProto builds the workflow request that pushes this Update's | ||
| // merged-into-DB state to a Site. The persisted `vpc` is the source of | ||
| // the wire fields because the handler has already merged the request's | ||
| // (sparse) update fields into the entity by the time this is called; | ||
| // sending the post-merge state matches the pre-existing handler | ||
| // behaviour and keeps unchanged fields populated. The handler may | ||
| // further set `DefaultNvlinkLogicalPartitionId` on the returned | ||
| // request when the API request changes that field. | ||
| func (asur APIVpcUpdateRequest) ToProto(vpc *cdbm.Vpc) *cwssaws.VpcUpdateRequest { | ||
| vpcProto := vpc.ToProto() | ||
| return &cwssaws.VpcUpdateRequest{ | ||
| Id: vpcProto.Id, | ||
| NetworkSecurityGroupId: vpcProto.NetworkSecurityGroupId, | ||
| Metadata: vpcProto.Metadata, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve explicit NSG clears in the update proto.
ToProto currently serializes NetworkSecurityGroupId only from the post-merge DB vpc. On the networkSecurityGroupId: "" path, the handler clears that DB field before calling this helper, so the workflow request gets nil instead of "" and the Site never sees the detach. This helper needs to let the request override the DB value when asur.NetworkSecurityGroupID != nil.
Suggested fix
func (asur APIVpcUpdateRequest) ToProto(vpc *cdbm.Vpc) *cwssaws.VpcUpdateRequest {
vpcProto := vpc.ToProto()
- return &cwssaws.VpcUpdateRequest{
+ req := &cwssaws.VpcUpdateRequest{
Id: vpcProto.Id,
NetworkSecurityGroupId: vpcProto.NetworkSecurityGroupId,
Metadata: vpcProto.Metadata,
}
+ if asur.NetworkSecurityGroupID != nil {
+ req.NetworkSecurityGroupId = asur.NetworkSecurityGroupID
+ }
+ return req
}Based on learnings: Per-API-request → proto request conversion lives on the API request type.
🤖 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 `@api/pkg/api/model/vpc.go` around lines 232 - 247, The current
APIVpcUpdateRequest.ToProto always takes NetworkSecurityGroupId from the merged
DB vpc, which loses explicit clears when asur.NetworkSecurityGroupID is set to
an empty string; modify APIVpcUpdateRequest.ToProto so it prefers the API
request override when asur.NetworkSecurityGroupID != nil (using its value, which
may be ""), and falls back to vpc.ToProto().NetworkSecurityGroupId only when
asur.NetworkSecurityGroupID is nil; update the returned
cwssaws.VpcUpdateRequest.NetworkSecurityGroupId accordingly.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
hwadekar-nv
reviewed
May 7, 2026
| } else { | ||
| vpc.NVLinkLogicalPartitionID = nil | ||
| } | ||
| if proto.Metadata != nil { |
Contributor
There was a problem hiding this comment.
Can we check name from Metadata in case if proto.Name is nill or empty?
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
2 participants
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.
Description
Implements a layered convention for proto-conversion methods that the rest of the proto handling rollout will adopt. This was already done for
Expected*components, but VPC had some additional bits that I was fiddling with (I did #501 but that was some stuff to be shared acrossvpc,vpcprefix, andvpcpeering).All said, this splits proto conversion into clear layers, and applies to
vpchandlers:ToProto/FromProtoon the DB entity.ToProtoon each API request type for request-shape conversion.AGENTS.mdhas been further updated to document this layering.Tests added!
Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes