refactor: factor object metadata fields in APICarbideObjectMetadata#469
Draft
ianderson-nvidia wants to merge 1 commit intoNVIDIA:mainfrom
Draft
refactor: factor object metadata fields in APICarbideObjectMetadata#469ianderson-nvidia wants to merge 1 commit intoNVIDIA:mainfrom
ianderson-nvidia wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughRefactors VPC API shapes by consolidating identity fields ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ianderson-nvidia
force-pushed
the
update_vpc_metadata
branch
from
6a165e0 to
7dc7ae9
Compare
![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: 3
🧹 Nitpick comments (2)
api/pkg/api/handler/vpc_test.go (1)
1016-1018: ⚡ Quick winAssert metadata label values, not just map size.
These checks still go green if the handler drops a label, rewrites a value, or returns the wrong map with the same count. The GET paths also do not verify labels at all after the field move.
Suggested tightening of the assertions
- if tt.args.reqData.Metadata.Labels != nil { - assert.Equal(t, len(rst.Metadata.Labels), len(tt.args.reqData.Metadata.Labels)) - } + if tt.args.reqData.Metadata.Labels != nil { + assert.Equal(t, tt.args.reqData.Metadata.Labels, rst.Metadata.Labels) + }assert.Equal(t, *rst.Metadata.Name, tt.args.reqVPC.Name) assert.Equal(t, rst.Metadata.Description, tt.args.reqVPC.Description) + assert.Equal(t, tt.args.reqVPC.Labels, rst.Metadata.Labels)if tt.wantFirstEntry != nil { assert.Equal(t, tt.wantFirstEntry.Name, *resp[0].Metadata.Name) + assert.Equal(t, tt.wantFirstEntry.Labels, resp[0].Metadata.Labels) }Also applies to: 1591-1605, 2226-2228, 2933-2935
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/vpc_test.go` around lines 1016 - 1018, The tests currently only compare label map sizes (assert.Equal on len(...)) which misses dropped/changed keys or values; update the assertions to compare the actual maps of labels (e.g., assert.Equal or DeepEqual between tt.args.reqData.Metadata.Labels and rst.Metadata.Labels) so keys and values are validated, and add equivalent checks for the GET-path assertions after the field move; apply this change at the occurrences around the shown snippet and the other locations noted (lines ~1591-1605, ~2226-2228, ~2933-2935) to ensure full label value equality is asserted rather than just map length.api/pkg/api/model/carbide_object_metadata_test.go (1)
30-214: ⚡ Quick winAdd direct label-validation cases to the shared metadata tests.
ValidateOnCreateandValidateOnUpdateboth callutil.ValidateLabels, but this suite never asserts that path directly. That leaves the shared validator under-tested outside of VPC handler coverage.Example cases to add
{ name: "description at the 1024 char limit", metadata: APICarbideObjectMetadata{ Name: cdb.GetStrPtr("test-name"), Description: cdb.GetStrPtr(strings.Repeat("a", 1024)), }, wantErr: false, }, + { + name: "invalid label key is rejected", + metadata: APICarbideObjectMetadata{ + Name: cdb.GetStrPtr("test-name"), + Labels: map[string]string{"": "prod"}, + }, + wantErr: true, + },{ name: "labels-only update", metadata: APICarbideObjectMetadata{ Labels: map[string]string{"env": "prod"}, }, wantErr: false, }, + { + name: "invalid label key on update is rejected", + metadata: APICarbideObjectMetadata{ + Labels: map[string]string{"": "prod"}, + }, + wantErr: true, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/model/carbide_object_metadata_test.go` around lines 30 - 214, Add direct label-validation test cases to TestAPICarbideObjectMetadata_ValidateOnCreate and TestAPICarbideObjectMetadata_ValidateOnUpdate so util.ValidateLabels behavior is exercised: include cases with invalid labels (e.g., empty key, empty value, key with leading/trailing whitespace, and overly long key/value) that should produce wantErr=true and valid label maps (simple key/value and multiple labels) that should produce wantErr=false; reference APICarbideObjectMetadata, ValidateOnCreate, ValidateOnUpdate, and util.ValidateLabels when adding these table-driven entries so the shared label validation path is explicitly covered.
🤖 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/pkg/api/handler/instance_test.go`:
- Line 6768: The test dereferences nested VPC fields directly (assert.Equal(t,
*tt.expectedVpcName, *rst.Vpc.Metadata.Name)) which can panic if rst.Vpc,
rst.Vpc.Metadata or rst.Vpc.Metadata.Name are nil; add explicit require.NotNil
checks for tt.expectedVpcName, rst.Vpc, rst.Vpc.Metadata and
rst.Vpc.Metadata.Name before the assert.Equal call so the test fails with clear
errors instead of panicking. Locate the assertion referencing tt.expectedVpcName
and rst.Vpc.Metadata.Name in instance_test.go and prepend require.NotNil(t,
tt.expectedVpcName), require.NotNil(t, rst.Vpc), require.NotNil(t,
rst.Vpc.Metadata) and require.NotNil(t, rst.Vpc.Metadata.Name) (using the same
testing package as other tests) then keep the existing assert.Equal afterwards.
In `@api/pkg/api/handler/subnet_test.go`:
- Line 1060: The test is comparing pointer and non-pointer strings; update the
assertions that use tc.expectedVpcName and resp[0].Vpc.Metadata.Name (and the
similar assertion around the other occurrence) to first nil-check both pointers
and then compare the dereferenced values: ensure tc.expectedVpcName != nil and
resp[0].Vpc.Metadata.Name != nil before asserting assert.Equal(t,
*tc.expectedVpcName, *resp[0].Vpc.Metadata.Name) (or use assert.Nil/NotNil where
appropriate) so types match and nil cases are handled.
In `@api/pkg/api/handler/vpcprefix_test.go`:
- Line 1009: The test compares tc.expectedVpcName (a string pointer) to
resp[0].Vpc.Metadata.Name and rsp.Vpc.Metadata.Name (both *string) which can be
nil; update the assertions to safely handle nil pointers by first checking that
resp[0].Vpc.Metadata and resp[0].Vpc.Metadata.Name (and rsp.Vpc.Metadata /
rsp.Vpc.Metadata.Name) are non-nil (or use require.NotNil) and then compare the
dereferenced value to *tc.expectedVpcName (or reverse: compare
tc.expectedVpcName to pointer via aws.StringValue/utility function) so the types
match and avoid panics; adjust both references where they appear:
tc.expectedVpcName, resp[0].Vpc.Metadata.Name and rsp.Vpc.Metadata.Name.
---
Nitpick comments:
In `@api/pkg/api/handler/vpc_test.go`:
- Around line 1016-1018: The tests currently only compare label map sizes
(assert.Equal on len(...)) which misses dropped/changed keys or values; update
the assertions to compare the actual maps of labels (e.g., assert.Equal or
DeepEqual between tt.args.reqData.Metadata.Labels and rst.Metadata.Labels) so
keys and values are validated, and add equivalent checks for the GET-path
assertions after the field move; apply this change at the occurrences around the
shown snippet and the other locations noted (lines ~1591-1605, ~2226-2228,
~2933-2935) to ensure full label value equality is asserted rather than just map
length.
In `@api/pkg/api/model/carbide_object_metadata_test.go`:
- Around line 30-214: Add direct label-validation test cases to
TestAPICarbideObjectMetadata_ValidateOnCreate and
TestAPICarbideObjectMetadata_ValidateOnUpdate so util.ValidateLabels behavior is
exercised: include cases with invalid labels (e.g., empty key, empty value, key
with leading/trailing whitespace, and overly long key/value) that should produce
wantErr=true and valid label maps (simple key/value and multiple labels) that
should produce wantErr=false; reference APICarbideObjectMetadata,
ValidateOnCreate, ValidateOnUpdate, and util.ValidateLabels when adding these
table-driven entries so the shared label validation path is explicitly covered.
🪄 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: 3b515625-2d9a-4cde-9bf6-3c75966e21c0
📒 Files selected for processing (10)
api/pkg/api/handler/instance_test.goapi/pkg/api/handler/subnet_test.goapi/pkg/api/handler/vpc.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/handler/vpcprefix_test.goapi/pkg/api/model/carbide_object_metadata.goapi/pkg/api/model/carbide_object_metadata_test.goapi/pkg/api/model/nvlinklogicalpartition_test.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.go
ianderson-nvidia
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/pkg/api/model/carbide_object_metadata.go (2)
38-54: 💤 Low valueRedundant conditional guard after
Requiredvalidation.Line 43's
validation.When(m.Name != nil, ...)is superfluous. Thevalidation.Requiredon line 41 will fail-fast whenNameis nil, preventing theWhenblock from ever executing on a nil value. This does not cause incorrect behavior but adds unnecessary cognitive overhead.♻️ Proposed simplification
if err := validation.ValidateStruct(&m, validation.Field(&m.Name, validation.Required.Error(validationErrorStringLength), validation.By(util.ValidateNameCharacters), - validation.When(m.Name != nil, - validation.Length(2, 256).Error(validationErrorStringLength)), + validation.Length(2, 256).Error(validationErrorStringLength), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/model/carbide_object_metadata.go` around lines 38 - 54, In ValidateOnCreate, remove the redundant validation.When(m.Name != nil, ...) guard around the Length(2, 256) check for m.Name because validation.Required already enforces non-nil; apply the Length(2, 256).Error(validationErrorStringLength) directly to the m.Name Field alongside validation.Required and validation.By(util.ValidateNameCharacters) so the Name length is validated when present without the unnecessary When wrapper.
75-83: ⚡ Quick winConsider accepting a pointer parameter for efficiency.
The
cdbm.Vpcstruct is substantial (perdb/pkg/db/model/vpc.go—contains numerous fields including relations, timestamps, and nested structs). Passing by value copies the entire struct on each call. Accepting a pointer would avoid this allocation overhead.♻️ Proposed signature change
-func NewAPICarbideObjectMetadataFromVpc(v cdbm.Vpc) APICarbideObjectMetadata { +func NewAPICarbideObjectMetadataFromVpc(v *cdbm.Vpc) APICarbideObjectMetadata { return APICarbideObjectMetadata{ Name: &v.Name, Description: v.Description, Labels: v.Labels, } }This change would require updating call sites in
vpc.go(lines 265 and 358) to pass&dbVpcanddbVpcrespectively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/model/carbide_object_metadata.go` around lines 75 - 83, Change NewAPICarbideObjectMetadataFromVpc to accept a pointer (*cdbm.Vpc) instead of a value to avoid copying the large Vpc struct: update its signature to NewAPICarbideObjectMetadataFromVpc(v *cdbm.Vpc) and keep the body using v.Name, v.Description, v.Labels (pointer deref is implicit). Then update all call sites that currently pass a cdbm.Vpc value (e.g., places passing dbVpc) to pass the address (&dbVpc) or propagate a *cdbm.Vpc where available, and run/update any dependent tests or callers that assumed a value parameter.
🤖 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/pkg/api/model/carbide_object_metadata.go`:
- Around line 38-54: In ValidateOnCreate, remove the redundant
validation.When(m.Name != nil, ...) guard around the Length(2, 256) check for
m.Name because validation.Required already enforces non-nil; apply the Length(2,
256).Error(validationErrorStringLength) directly to the m.Name Field alongside
validation.Required and validation.By(util.ValidateNameCharacters) so the Name
length is validated when present without the unnecessary When wrapper.
- Around line 75-83: Change NewAPICarbideObjectMetadataFromVpc to accept a
pointer (*cdbm.Vpc) instead of a value to avoid copying the large Vpc struct:
update its signature to NewAPICarbideObjectMetadataFromVpc(v *cdbm.Vpc) and keep
the body using v.Name, v.Description, v.Labels (pointer deref is implicit). Then
update all call sites that currently pass a cdbm.Vpc value (e.g., places passing
dbVpc) to pass the address (&dbVpc) or propagate a *cdbm.Vpc where available,
and run/update any dependent tests or callers that assumed a value parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aa356060-e2c6-4856-ae47-785ddaaba10a
📒 Files selected for processing (10)
api/pkg/api/handler/instance_test.goapi/pkg/api/handler/subnet_test.goapi/pkg/api/handler/vpc.goapi/pkg/api/handler/vpc_test.goapi/pkg/api/handler/vpcprefix_test.goapi/pkg/api/model/carbide_object_metadata.goapi/pkg/api/model/carbide_object_metadata_test.goapi/pkg/api/model/nvlinklogicalpartition_test.goapi/pkg/api/model/vpc.goapi/pkg/api/model/vpc_test.go
✅ Files skipped from review due to trivial changes (5)
- api/pkg/api/handler/vpcprefix_test.go
- api/pkg/api/model/carbide_object_metadata_test.go
- api/pkg/api/handler/subnet_test.go
- api/pkg/api/handler/vpc.go
- api/pkg/api/handler/vpc_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/pkg/api/model/nvlinklogicalpartition_test.go
- api/pkg/api/model/vpc_test.go
ianderson-nvidia
changed the title
ianderson-nvidia
changed the title
ianderson-nvidia
force-pushed
the
update_vpc_metadata
branch
from
7dc7ae9 to
9ea5b63
Compare
Mirror carbide-api gRPC `Metadata` message on the REST side by introducing a shared `APICarbideObjectMetadata`. For now this is only used in VPC related API calls. Clients that read or set these three fields need to migrate to to the new API. The OpenAPI spec and generated clients will need to regenerated after this commit. Signed-off-by: Ian Anderson <ianderson@nvidia.com>
ianderson-nvidia
force-pushed
the
update_vpc_metadata
branch
from
9ea5b63 to
2c7333b
Compare
Contributor
|
@ianderson-nvidia Curious why we want to make this switch. Did we receive any particular feedback about it? These fields are intentionally made first class attributes in the REST API. The changes are breaking and we normally provide a 3 - 6 months deprecation period for breaking changes. During this period we have to support both ways to reading and writing the attributes. We can also keep the gRPC API different in this regard. |
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
Mirror carbide-api gRPC
Metadatamessage on the REST side by introducing a sharedAPICarbideObjectMetadata. For now this is only used in VPC related API calls.Clients that read or set these three fields need to migrate to to the new API. The OpenAPI spec and generated clients will need to regenerated after this commit.
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes