chore: Apply the new FromProto/ToProto modeling to MachineCapability#569
Open
chet wants to merge 1 commit into
Open
chore: Apply the new FromProto/ToProto modeling to MachineCapability#569chet wants to merge 1 commit into
chet wants to merge 1 commit into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMigrates MachineCapability fields from untyped strings to typed enums with ToProto/FromProto/Validate, updates DB model and DAO validation, replaces string-pointer helpers with typed-pointer helpers, updates handlers/workflow/tests to use typed enums, adds pointer/uint32 conversion helpers, and documents the conventions. ChangesType Safety Migration for Machine Capabilities
🎯 4 (Complex) | ⏱️ ~60 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 |
chet
force-pushed
the
proto-machinecapability-receivers
branch
from
61479e3 to
e133404
Compare
🔐 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-22 06:51:11 UTC | Commit: e133404 |
![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: 4
🧹 Nitpick comments (4)
workflow/pkg/activity/instancetype/instancetype_test.go (1)
574-583: 💤 Low valueManual device type mapping is acceptable for test verification, but consider using
ToProtofor consistency.The explicit switch statement adequately covers the expected test fixture values (DPU, NVLink) and appropriately fails fast on unexpected types. However, since the production code now uses typed enums with
ToProtomethods, leveraging that method here would reduce duplication and ensure the test validates the actual conversion logic.This is a minor observation and not blocking.
Alternative using ToProto
-var protoDeviceType cwssaws.MachineCapabilityDeviceType -switch *cloudCaps[i].DeviceType { -case cdbm.MachineCapabilityDeviceTypeDPU: - protoDeviceType = cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_DPU -case cdbm.MachineCapabilityDeviceTypeNVLink: - protoDeviceType = cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_NVLINK -default: - t.Fatalf("unsupported DeviceType %q in test fixture", *cloudCaps[i].DeviceType) -} -assert.Equal(t, *siteCaps[i].DeviceType, protoDeviceType) +assert.Equal(t, *siteCaps[i].DeviceType, cloudCaps[i].DeviceType.ToProto())🤖 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 `@workflow/pkg/activity/instancetype/instancetype_test.go` around lines 574 - 583, The test currently maps cloudCaps[i].DeviceType to protoDeviceType via an explicit switch and t.Fatalf on unknown values; instead call the production conversion method (e.g., the enum's ToProto) to produce the proto value and compare that to siteCaps[i].DeviceType to avoid duplication — replace the switch that sets protoDeviceType with a call like cloudCaps[i].DeviceType.ToProto() (or the appropriate ToProto function for the enum) and keep the assert.Equal against siteCaps[i].DeviceType, preserving failure behavior if ToProto returns an unexpected/zero value.workflow/pkg/activity/instancetype/instancetype.go (1)
365-376: ⚡ Quick winConsider adding validation for consistency with
UpdateInstanceTypeInCloud.In
UpdateInstanceTypeInCloud(lines 187-191), the pattern isFromProtofollowed byValidate()to catch invalid proto enum values. Here,MachineCapabilityType.FromProtoandMachineCapabilityDeviceType.FromProtoare called without subsequent validation. If the proto contains an unrecognized enum value, it may silently produce an empty or unexpected type that persists to the database.Per the PR objectives: "callers are expected to run Validate alongside FromProto." Consider either:
- Creating a temporary
MachineCapabilitystruct, callingFromProto+Validate()as inUpdateInstanceTypeInCloud, or- Validating the converted
capTypeis non-empty before proceeding.Proposed validation addition
var capType cdbm.MachineCapabilityType capType.FromProto(controllerCap.GetCapabilityType()) +if capType == "" { + return nil, fmt.Errorf("unsupported capability type for capability at index %d", idx) +}🤖 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 `@workflow/pkg/activity/instancetype/instancetype.go` around lines 365 - 376, The code calls MachineCapabilityDeviceType.FromProto and MachineCapabilityType.FromProto on controllerCap without validating the results; update this to match UpdateInstanceTypeInCloud by constructing a temporary MachineCapability (or temporary variables) and after calling FromProto on controllerCap.DeviceType and capType call Validate() (or at minimum check the converted capType/dt is non-empty) and only assign deviceType or use capType if Validate() succeeds; reference MachineCapabilityDeviceType, MachineCapabilityType, FromProto(), Validate(), and UpdateInstanceTypeInCloud to mirror its pattern.workflow/pkg/activity/machine/machine.go (2)
867-873: ⚡ Quick winSimplify device type initialization for Network capabilities.
The same verbose initialization pattern used for GPU capabilities appears here. Apply the same simplification to maintain consistency and improve readability.
♻️ Proposed refactor
- // Set the device type to DPU if it's a DPU network capability - var deviceType *cdbm.MachineCapabilityDeviceType - dtEmpty := cdbm.MachineCapabilityDeviceType("") - deviceType = &dtEmpty + var deviceType *cdbm.MachineCapabilityDeviceType if netCap.DeviceType != nil && *netCap.DeviceType == cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_DPU { dt := cdbm.MachineCapabilityDeviceTypeDPU deviceType = &dt }This leaves
deviceTypeasnilwhen the network capability is not DPU-backed, which is more idiomatic for optional fields in Go.🤖 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 `@workflow/pkg/activity/machine/machine.go` around lines 867 - 873, The deviceType initialization is verbose and should be simplified to be nil by default and only set when the network capability is DPU; update the logic around the deviceType variable (currently using dtEmpty, dt and checking netCap.DeviceType) so that deviceType (type *cdbm.MachineCapabilityDeviceType) starts as nil and is assigned a pointer to cdbm.MachineCapabilityDeviceTypeDPU only when netCap.DeviceType != nil && *netCap.DeviceType == cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_DPU, removing the dtEmpty/dt temporary variables for cleaner, idiomatic optional handling.
792-799: ⚡ Quick winSimplify device type initialization for GPU capabilities.
The current pattern initializes
deviceTypeto point to an empty string""and then conditionally reassigns it. For optional pointer fields,nilis more idiomatic to indicate absence. Consider simplifying:♻️ Proposed refactor
- // Set the device type to NVLink if it's an NVLink GPU capability - var deviceType *cdbm.MachineCapabilityDeviceType - dtEmpty := cdbm.MachineCapabilityDeviceType("") - deviceType = &dtEmpty + var deviceType *cdbm.MachineCapabilityDeviceType if gpuCap.DeviceType != nil && *gpuCap.DeviceType == cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_NVLINK { dt := cdbm.MachineCapabilityDeviceTypeNVLink deviceType = &dt }This leaves
deviceTypeasnilwhen the GPU is not NVLink, which more clearly expresses the absence of a device type specification.🤖 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 `@workflow/pkg/activity/machine/machine.go` around lines 792 - 799, The code sets deviceType to point to an empty string then conditionally reassigns it; instead make deviceType a nil pointer by default and only assign it when gpuCap.DeviceType indicates NVLink. Specifically, replace the current deviceType initialization so deviceType is declared as var deviceType *cdbm.MachineCapabilityDeviceType (nil) and only set it to &dt where dt := cdbm.MachineCapabilityDeviceTypeNVLink when gpuCap.DeviceType != nil && *gpuCap.DeviceType == cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_NVLINK; leave deviceType nil otherwise to represent absence.
🤖 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 `@api/pkg/api/handler/instance_test.go`:
- Line 708: Replace the opaque pointer casts to
*cdbm.MachineCapabilityDeviceType in the instance tests by creating typed local
variables of type cdbm.MachineCapabilityDeviceType and passing their addresses;
specifically, in the TestBuildMachineCapability calls (e.g. the call that
currently uses (*cdbm.MachineCapabilityDeviceType)(cdb.GetStrPtr("")) and the
one using (*cdbm.MachineCapabilityDeviceType)(cdb.GetStrPtr("DPU"))), declare
locals such as emptyDT := cdbm.MachineCapabilityDeviceType("") and dpuDT :=
cdbm.MachineCapabilityDeviceType("DPU") and pass &emptyDT / &dpuDT to the
TestBuildMachineCapability invocations (and any other test helpers using
MachineCapabilityDeviceType) instead of the pointer casts.
In `@api/pkg/api/handler/instancetype.go`:
- Around line 1065-1067: The loop building existingMacCapMap captures the loop
variable address (&mc) causing pointer aliasing; instead take a stable pointer
for each element (e.g. use the slice element by index or assign mc to a new
local variable and take its address) when populating existingMacCapMap so each
map value points to the correct MachineCapability; update the loop that
references existingMcs and ensure MapKey() is called on the element used for the
stable pointer so subsequent MachineCapabilitiesEqual(...) and DeleteByID(...)
operations use the correct objects.
In `@common/pkg/util/converter.go`:
- Around line 35-48: Add unit tests for Uint32PtrToIntPtr in
common/pkg/util/converter.go: cover nil input (expect nil), zero value
(uint32(0) -> int(0)), a typical value (e.g., uint32(42) -> int(42)), and the
extreme uint32 max (math.MaxUint32 -> int(math.MaxUint32) on 64-bit builds). Put
tests in a new converter_test.go (or extend existing) and use table-driven
subtests asserting pointer nilness and dereferenced int equality for each case;
reference the function name Uint32PtrToIntPtr when locating the code to test.
In `@db/pkg/db/model/machinecapability.go`:
- Around line 501-503: The DAO currently only checks for a non-empty input.Type
at the machine_capability persistence points (the validation blocks around
input.Type at lines ~501–503 and ~844–846); change these checks to enforce
membership in the canonical capability type enum by validating input.Type
against the project's allowed capability constants (or a new helper
IsValidCapabilityType/AllowedCapabilityTypes slice) and return a descriptive
error (e.g., "invalid capability.type: %q") if it is not present; apply this
change in both validation sites and centralize the allowed-values list/function
so both machine capability DAO entry points use the same validation logic and
update any related tests to cover invalid-but-non-empty values.
---
Nitpick comments:
In `@workflow/pkg/activity/instancetype/instancetype_test.go`:
- Around line 574-583: The test currently maps cloudCaps[i].DeviceType to
protoDeviceType via an explicit switch and t.Fatalf on unknown values; instead
call the production conversion method (e.g., the enum's ToProto) to produce the
proto value and compare that to siteCaps[i].DeviceType to avoid duplication —
replace the switch that sets protoDeviceType with a call like
cloudCaps[i].DeviceType.ToProto() (or the appropriate ToProto function for the
enum) and keep the assert.Equal against siteCaps[i].DeviceType, preserving
failure behavior if ToProto returns an unexpected/zero value.
In `@workflow/pkg/activity/instancetype/instancetype.go`:
- Around line 365-376: The code calls MachineCapabilityDeviceType.FromProto and
MachineCapabilityType.FromProto on controllerCap without validating the results;
update this to match UpdateInstanceTypeInCloud by constructing a temporary
MachineCapability (or temporary variables) and after calling FromProto on
controllerCap.DeviceType and capType call Validate() (or at minimum check the
converted capType/dt is non-empty) and only assign deviceType or use capType if
Validate() succeeds; reference MachineCapabilityDeviceType,
MachineCapabilityType, FromProto(), Validate(), and UpdateInstanceTypeInCloud to
mirror its pattern.
In `@workflow/pkg/activity/machine/machine.go`:
- Around line 867-873: The deviceType initialization is verbose and should be
simplified to be nil by default and only set when the network capability is DPU;
update the logic around the deviceType variable (currently using dtEmpty, dt and
checking netCap.DeviceType) so that deviceType (type
*cdbm.MachineCapabilityDeviceType) starts as nil and is assigned a pointer to
cdbm.MachineCapabilityDeviceTypeDPU only when netCap.DeviceType != nil &&
*netCap.DeviceType ==
cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_DPU, removing
the dtEmpty/dt temporary variables for cleaner, idiomatic optional handling.
- Around line 792-799: The code sets deviceType to point to an empty string then
conditionally reassigns it; instead make deviceType a nil pointer by default and
only assign it when gpuCap.DeviceType indicates NVLink. Specifically, replace
the current deviceType initialization so deviceType is declared as var
deviceType *cdbm.MachineCapabilityDeviceType (nil) and only set it to &dt where
dt := cdbm.MachineCapabilityDeviceTypeNVLink when gpuCap.DeviceType != nil &&
*gpuCap.DeviceType ==
cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_NVLINK; leave
deviceType nil otherwise to represent absence.
🪄 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: 77a18393-b2fc-4065-b45f-763c31ca8578
📒 Files selected for processing (27)
AGENTS.mdapi/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/instancetype.goapi/pkg/api/handler/instancetype_test.goapi/pkg/api/handler/machine.goapi/pkg/api/handler/machine_test.goapi/pkg/api/handler/machinecapability_test.goapi/pkg/api/handler/stats.goapi/pkg/api/handler/stats_test.goapi/pkg/api/handler/util/common/common.goapi/pkg/api/handler/util/common/common_test.goapi/pkg/api/handler/util/common/testing.goapi/pkg/api/model/instancetype_test.goapi/pkg/api/model/machinecapability.gocommon/pkg/util/converter.godb/pkg/db/model/machine_test.godb/pkg/db/model/machinecapability.godb/pkg/db/model/machinecapability_test.godb/pkg/db/util.goworkflow/pkg/activity/instancetype/instancetype.goworkflow/pkg/activity/instancetype/instancetype_test.goworkflow/pkg/activity/machine/machine.goworkflow/pkg/activity/machine/machine_test.goworkflow/pkg/util/conv.go
💤 Files with no reviewable changes (1)
- workflow/pkg/util/conv.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workflow/pkg/activity/instancetype/instancetype.go (1)
181-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
(Type, Name)as the capability identity here.Both flows still key/dedupe capabilities by
Nameonly, but the new model now defines capability identity via(Type, Name)and exposesMapKey()for that purpose.GPU/fooandNetwork/foowill collide here: the add path rejects a valid second entry, and the update path can overwrite or delete the wrong capability during sync.Suggested direction
- if controllerCapMap[controllerCap.GetName()] != nil { + key := machineCap.MapKey() + if controllerCapMap[key] != nil { return errors.New("site returned multiple capabilities with the same name") } - - macCapName := machineCap.Name - controllerCapMap[macCapName] = machineCap + controllerCapMap[key] = machineCap ... - macCapName := cloudCap.Name - cloudCapMap[macCapName] = &cloudCap + cloudCapMap[cloudCap.MapKey()] = &cloudCapApply the same identity on the create path after converting the incoming proto capability, rather than deduping on
GetName()alone.Also applies to: 198-200, 345-379
🤖 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 `@workflow/pkg/activity/instancetype/instancetype.go` around lines 181 - 194, The code currently dedupes and keys capabilities by machineCap.Name; instead use the canonical identity returned by machineCap.MapKey(). After converting the proto into a cdbm.MachineCapability (using machineCap.FromProto and machineCap.Validate) compute key := machineCap.MapKey() and use controllerCapMap[key] for the duplicate check, map assignment (controllerCapMap[key] = machineCap), and any removals/updates; apply the same change to the other capability-handling blocks (the create/add path and the update/sync path that also reference controllerCapMap and GetDesiredCapabilities) so capabilities are identified by (Type, Name) rather than Name alone.
🧹 Nitpick comments (1)
api/pkg/api/handler/machinecapability_test.go (1)
69-91: ⚡ Quick winUnify
MachineCapabilityDeviceTypepointer construction in machinecapability fixturesIn
api/pkg/api/handler/machinecapability_test.go(lines 69-91), the fixtures build*cdbm.MachineCapabilityDeviceTypevia(*cdbm.MachineCapabilityDeviceType)(cdb.GetStrPtr(...)), but elsewhere (e.g., around line 222) the codebase already uses the generic helpercdb.Ptr(cdbm.MachineCapabilityDeviceType(...)). Usecdb.Ptr(...)consistently to avoid the noisy explicit cast.Also, the empty-string device type is already used as test data in
db/pkg/db/model/machinecapability_test.go, so clarify (via a short comment) what""means there (e.g., “unspecified”) rather than switching tonilwithout documenting the intended semantics.🤖 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/handler/machinecapability_test.go` around lines 69 - 91, Replace the noisy casts like (*cdbm.MachineCapabilityDeviceType)(cdb.GetStrPtr("")) passed into TestBuildMachineCapability with the canonical helper cdb.Ptr(cdbm.MachineCapabilityDeviceType("")) (and similar for "DPU" or other values) so callers use cdb.Ptr(...) consistently; update each TestBuildMachineCapability call in this snippet to pass cdb.Ptr(cdbm.MachineCapabilityDeviceType(...)) instead of the explicit cast, and add a short inline comment next to the empty-string value explaining that "" represents “unspecified” (not nil) to match the semantics used in db/pkg/db/model/machinecapability_test.go.
🤖 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 `@db/pkg/db/model/machinecapability.go`:
- Around line 346-362: MachineCapability.Validate currently only checks Type and
Name; extend it to enforce cross-field invariants by adding a custom validator
(validation.By) that inspects the whole MachineCapability (mc) and returns an
error when illegal combinations are present; specifically reject Network with
NVLink, GPU with DPU, and any non-InfiniBand Type that has InactiveDevices set
(and conversely require InactiveDevices only for InfiniBand when appropriate).
Implement this by adding an extra validation.Field(&mc, validation.By(func(value
any) error { mc := value.(*MachineCapability); check combinations and return
validation.Err... / fmt.Errorf(...) }) ) or equivalent inside
MachineCapability.Validate so those invalid cross-field shapes are rejected
alongside the existing Type/Name checks.
---
Outside diff comments:
In `@workflow/pkg/activity/instancetype/instancetype.go`:
- Around line 181-194: The code currently dedupes and keys capabilities by
machineCap.Name; instead use the canonical identity returned by
machineCap.MapKey(). After converting the proto into a cdbm.MachineCapability
(using machineCap.FromProto and machineCap.Validate) compute key :=
machineCap.MapKey() and use controllerCapMap[key] for the duplicate check, map
assignment (controllerCapMap[key] = machineCap), and any removals/updates; apply
the same change to the other capability-handling blocks (the create/add path and
the update/sync path that also reference controllerCapMap and
GetDesiredCapabilities) so capabilities are identified by (Type, Name) rather
than Name alone.
---
Nitpick comments:
In `@api/pkg/api/handler/machinecapability_test.go`:
- Around line 69-91: Replace the noisy casts like
(*cdbm.MachineCapabilityDeviceType)(cdb.GetStrPtr("")) passed into
TestBuildMachineCapability with the canonical helper
cdb.Ptr(cdbm.MachineCapabilityDeviceType("")) (and similar for "DPU" or other
values) so callers use cdb.Ptr(...) consistently; update each
TestBuildMachineCapability call in this snippet to pass
cdb.Ptr(cdbm.MachineCapabilityDeviceType(...)) instead of the explicit cast, and
add a short inline comment next to the empty-string value explaining that ""
represents “unspecified” (not nil) to match the semantics used in
db/pkg/db/model/machinecapability_test.go.
🪄 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: 3c875fdd-fd50-48cc-a1cc-9c40e5c082cc
📒 Files selected for processing (27)
AGENTS.mdapi/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/instancetype.goapi/pkg/api/handler/instancetype_test.goapi/pkg/api/handler/machine.goapi/pkg/api/handler/machine_test.goapi/pkg/api/handler/machinecapability_test.goapi/pkg/api/handler/stats.goapi/pkg/api/handler/stats_test.goapi/pkg/api/handler/util/common/common.goapi/pkg/api/handler/util/common/common_test.goapi/pkg/api/handler/util/common/testing.goapi/pkg/api/model/instancetype_test.goapi/pkg/api/model/machinecapability.gocommon/pkg/util/converter.godb/pkg/db/model/machine_test.godb/pkg/db/model/machinecapability.godb/pkg/db/model/machinecapability_test.godb/pkg/db/util.goworkflow/pkg/activity/instancetype/instancetype.goworkflow/pkg/activity/instancetype/instancetype_test.goworkflow/pkg/activity/machine/machine.goworkflow/pkg/activity/machine/machine_test.goworkflow/pkg/util/conv.go
💤 Files with no reviewable changes (1)
- workflow/pkg/util/conv.go
✅ Files skipped from review due to trivial changes (2)
- db/pkg/db/model/machine_test.go
- AGENTS.md
chet
force-pushed
the
proto-machinecapability-receivers
branch
from
e133404 to
815e091
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
workflow/pkg/activity/instancetype/instancetype.go (2)
177-200:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKey capability sync by
Type+Name, notNamealone.
MachineCapability.MapKey()makes(Type, Name)the canonical identity, but these paths still de-duplicate onNameonly. A site inventory containing the same name under different capability types will be rejected or diffed against the wrong row.Also applies to: 336-349
🤖 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 `@workflow/pkg/activity/instancetype/instancetype.go` around lines 177 - 200, The code is de-duplicating capabilities by Name only but MachineCapability.MapKey() uses (Type,Name) as the canonical identity; update both maps (controllerCapMap and cloudCapMap) to use machineCap.MapKey() (or cloudCap.MapKey()) as the map key instead of machineCap.Name/cloudCap.Name so items with the same Name but different Type are kept distinct; ensure you call MapKey() after constructing machineCap (in the controller loop where FromProto/Validate are used) and for cloudCaps (use the existing cloudCap value's MapKey()) wherever the map insertion currently uses Name.
351-390: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRoute the create path through
MachineCapability.FromProto()andValidate().
UpdateInstanceTypeInCloudalready uses the model-owned converter/validator, butAddInstanceTypeToCloudstill re-implements the mapping inline. That leaves the create path free to drift from the update path and miss future validation rules.As per coding guidelines, "Proto conversion methods: DB and API model types that round-trip with protobuf carry conversion as receiver methods, not free functions. Use `func (m *T) ToProto(...) *protoT` and `func (m *T) FromProto(p *protoT, ...)` as symmetric pairs defined together."♻️ Suggested simplification
- var count *int = nil - - if controllerCap.Count != nil { - c := int(controllerCap.GetCount()) - count = &c - } - - var inactiveDevices []int - if controllerCap.InactiveDevices != nil { - for _, d := range controllerCap.InactiveDevices.Items { - inactiveDevices = append(inactiveDevices, int(d)) - } - } - - var deviceType *cdbm.MachineCapabilityDeviceType - if controllerCap.DeviceType != nil { - var dt cdbm.MachineCapabilityDeviceType - dt.FromProto(*controllerCap.DeviceType) - if dt != "" { - deviceType = &dt - } - } - - var capType cdbm.MachineCapabilityType - capType.FromProto(controllerCap.GetCapabilityType()) + mc := &cdbm.MachineCapability{} + mc.FromProto(controllerCap, idx) + if err := mc.Validate(); err != nil { + return nil, fmt.Errorf("invalid machine capability from site: %w", err) + } _, err := macCapDAO.Create(ctx, tx, cdbm.MachineCapabilityCreateInput{ InstanceTypeID: &instanceType.ID, - Type: capType, - Name: controllerCap.GetName(), - Frequency: controllerCap.Frequency, - Capacity: controllerCap.Capacity, - Vendor: controllerCap.Vendor, - Cores: util.GetUint32PtrToIntPtr(controllerCap.Cores), - Threads: util.GetUint32PtrToIntPtr(controllerCap.Threads), - HardwareRevision: controllerCap.HardwareRevision, - InactiveDevices: inactiveDevices, - Count: count, - DeviceType: deviceType, - Index: idx, + Type: mc.Type, + Name: mc.Name, + Frequency: mc.Frequency, + Capacity: mc.Capacity, + Vendor: mc.Vendor, + Cores: mc.Cores, + Threads: mc.Threads, + HardwareRevision: mc.HardwareRevision, + InactiveDevices: mc.InactiveDevices, + Count: mc.Count, + DeviceType: mc.DeviceType, + Index: mc.Index, })🤖 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 `@workflow/pkg/activity/instancetype/instancetype.go` around lines 351 - 390, The inline mapping for controllerCap into macCapDAO.Create should be replaced by using the model's conversion and validation: instantiate a cdbm.MachineCapability, call its FromProto with controllerCap (the same conversion used by UpdateInstanceTypeInCloud), call its Validate() and surface the error if validation fails, then use the validated model's fields to build the MachineCapabilityCreateInput (or add a helper like MachineCapability.ToCreateInput) and pass that to macCapDAO.Create instead of duplicating field-by-field mapping in AddInstanceTypeToCloud; reference cdbm.MachineCapability, its FromProto and Validate methods, and macCapDAO.Create to locate the change.
🤖 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.
Outside diff comments:
In `@workflow/pkg/activity/instancetype/instancetype.go`:
- Around line 177-200: The code is de-duplicating capabilities by Name only but
MachineCapability.MapKey() uses (Type,Name) as the canonical identity; update
both maps (controllerCapMap and cloudCapMap) to use machineCap.MapKey() (or
cloudCap.MapKey()) as the map key instead of machineCap.Name/cloudCap.Name so
items with the same Name but different Type are kept distinct; ensure you call
MapKey() after constructing machineCap (in the controller loop where
FromProto/Validate are used) and for cloudCaps (use the existing cloudCap
value's MapKey()) wherever the map insertion currently uses Name.
- Around line 351-390: The inline mapping for controllerCap into
macCapDAO.Create should be replaced by using the model's conversion and
validation: instantiate a cdbm.MachineCapability, call its FromProto with
controllerCap (the same conversion used by UpdateInstanceTypeInCloud), call its
Validate() and surface the error if validation fails, then use the validated
model's fields to build the MachineCapabilityCreateInput (or add a helper like
MachineCapability.ToCreateInput) and pass that to macCapDAO.Create instead of
duplicating field-by-field mapping in AddInstanceTypeToCloud; reference
cdbm.MachineCapability, its FromProto and Validate methods, and macCapDAO.Create
to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bb85fab3-63a9-4fe1-88fe-00240a817b34
📒 Files selected for processing (28)
AGENTS.mdapi/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/instancetype.goapi/pkg/api/handler/instancetype_test.goapi/pkg/api/handler/machine.goapi/pkg/api/handler/machine_test.goapi/pkg/api/handler/machinecapability_test.goapi/pkg/api/handler/stats.goapi/pkg/api/handler/stats_test.goapi/pkg/api/handler/util/common/common.goapi/pkg/api/handler/util/common/common_test.goapi/pkg/api/handler/util/common/testing.goapi/pkg/api/model/instancetype_test.goapi/pkg/api/model/machinecapability.gocommon/pkg/util/converter.gocommon/pkg/util/converter_test.godb/pkg/db/model/machine_test.godb/pkg/db/model/machinecapability.godb/pkg/db/model/machinecapability_test.godb/pkg/db/util.goworkflow/pkg/activity/instancetype/instancetype.goworkflow/pkg/activity/instancetype/instancetype_test.goworkflow/pkg/activity/machine/machine.goworkflow/pkg/activity/machine/machine_test.goworkflow/pkg/util/conv.go
💤 Files with no reviewable changes (1)
- workflow/pkg/util/conv.go
✅ Files skipped from review due to trivial changes (2)
- common/pkg/util/converter_test.go
- AGENTS.md
chet
force-pushed
the
proto-machinecapability-receivers
branch
from
815e091 to
25cf7d0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
api/pkg/api/handler/instancetype_test.go (1)
721-721: ⚡ Quick winUse
nilfor absent device types in these fixtures.
MachineCapability.Validate()now treats device type as an enum-backed field. Seedingcdb.Ptr(cdbm.MachineCapabilityDeviceType(""))creates rows the production validation path should never produce, so these cases stop exercising the real “device type omitted” path.Proposed cleanup
- common.TestBuildMachineCapability(t, dbSession, nil, &it.ID, cdbm.MachineCapabilityTypeCPU, "Intel Xeon E5-2650v2", cdb.GetStrPtr("3.0Hz"), nil, nil, cdb.GetIntPtr(2), cdb.Ptr(cdbm.MachineCapabilityDeviceType("")), nil) + common.TestBuildMachineCapability(t, dbSession, nil, &it.ID, cdbm.MachineCapabilityTypeCPU, "Intel Xeon E5-2650v2", cdb.GetStrPtr("3.0Hz"), nil, nil, cdb.GetIntPtr(2), nil, nil)Apply the same change to the other three fixture calls in this block.
Also applies to: 784-784, 807-807, 819-819
🤖 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/handler/instancetype_test.go` at line 721, The fixtures are seeding an empty enum value for device type which bypasses the real "device type omitted" path; update the TestBuildMachineCapability calls (the one shown plus the three other calls in the same block) to pass nil instead of cdb.Ptr(cdbm.MachineCapabilityDeviceType("")) so MachineCapability.Validate() will exercise the omitted-device-type path; locate the TestBuildMachineCapability invocations that include cdb.Ptr(cdbm.MachineCapabilityDeviceType("")) and replace that argument with nil.
🤖 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 `@db/pkg/db/model/machinecapability.go`:
- Around line 324-330: The current logic drops presence of attrs.DeviceType when
dt.FromProto leaves dt == "" which makes an invalid proto enum indistinguishable
from omitted; instead always set deviceType = &dt when attrs.DeviceType != nil
(do not collapse to nil on unmapped values) so the presence is preserved and
MachineCapabilityDeviceType (populated via dt.FromProto) can be checked by
Validate() for unsupported values; update the block around attrs.DeviceType /
dt.FromProto / deviceType to assign the pointer regardless of dt == "" and rely
on Validate() to reject invalid enums.
- Around line 361-366: The DB model's Validate() currently only uses
validation.Required for MachineCapability.Name which allows whitespace-only
names; update the MachineCapability.Validate() (the validation of mc.Name) to
mirror APIMachineCapability.Validate() by trimming whitespace
(strings.TrimSpace) and then enforcing non-empty plus the same length and
character constraints used by APIMachineCapability (e.g., min/max length and
allowed chars) — implement this by replacing validation.Required for &mc.Name
with a validation.By or chained validators that first trims and then checks the
trimmed value for non-empty and the same length/char rules so whitespace-only
names are rejected at the DB model level.
In `@workflow/pkg/activity/instancetype/instancetype.go`:
- Around line 189-190: The inline error check using if err :=
machineCap.Validate(); err != nil should be refactored into two statements:
first call machineCap.Validate() and assign its result to a named variable
(err), then perform a separate if err != nil check and return the same formatted
error; update the block around machineCap.Validate() in instancetype.go (the
code that currently returns fmt.Errorf("failed to convert NICo machine
capability into MachineCapability: %w", err)) to use the split assignment and
conditional while preserving the existing error message and behavior.
In `@workflow/pkg/activity/machine/machine.go`:
- Around line 792-799: Replace the current nil-coercion logic that sets
deviceType to an empty cdbm.MachineCapabilityDeviceType in the machine
capability conversion code (around where gpuCap.DeviceType is read) with an
explicit switch over the incoming proto DeviceType values; handle the known case
cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_NVLINK by
setting deviceType to cdbm.MachineCapabilityDeviceTypeNVLink, add a default
branch that logs/warns about the unsupported/unknown proto value (including the
raw value) and leaves deviceType nil or returns an error as appropriate, and add
a TODO comment noting unsupported device types to satisfy the project guideline;
apply the same switch/default+TODO pattern to the analogous code path referenced
in the 867–873 block.
---
Nitpick comments:
In `@api/pkg/api/handler/instancetype_test.go`:
- Line 721: The fixtures are seeding an empty enum value for device type which
bypasses the real "device type omitted" path; update the
TestBuildMachineCapability calls (the one shown plus the three other calls in
the same block) to pass nil instead of
cdb.Ptr(cdbm.MachineCapabilityDeviceType("")) so MachineCapability.Validate()
will exercise the omitted-device-type path; locate the
TestBuildMachineCapability invocations that include
cdb.Ptr(cdbm.MachineCapabilityDeviceType("")) and replace that argument with
nil.
🪄 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: 7e2f8ce8-8874-413c-953e-2ba353ef260d
📒 Files selected for processing (28)
AGENTS.mdapi/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/instancetype.goapi/pkg/api/handler/instancetype_test.goapi/pkg/api/handler/machine.goapi/pkg/api/handler/machine_test.goapi/pkg/api/handler/machinecapability_test.goapi/pkg/api/handler/stats.goapi/pkg/api/handler/stats_test.goapi/pkg/api/handler/util/common/common.goapi/pkg/api/handler/util/common/common_test.goapi/pkg/api/handler/util/common/testing.goapi/pkg/api/model/instancetype_test.goapi/pkg/api/model/machinecapability.gocommon/pkg/util/converter.gocommon/pkg/util/converter_test.godb/pkg/db/model/machine_test.godb/pkg/db/model/machinecapability.godb/pkg/db/model/machinecapability_test.godb/pkg/db/util.goworkflow/pkg/activity/instancetype/instancetype.goworkflow/pkg/activity/instancetype/instancetype_test.goworkflow/pkg/activity/machine/machine.goworkflow/pkg/activity/machine/machine_test.goworkflow/pkg/util/conv.go
💤 Files with no reviewable changes (1)
- workflow/pkg/util/conv.go
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
chet
force-pushed
the
proto-machinecapability-receivers
branch
from
25cf7d0 to
8ac83a3
Compare
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
db/pkg/db/model/machinecapability_test.go (1)
1075-1085:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExpected-error update case does not currently assert an error.
This new negative-path case can false-pass because the shared
expectedErrorbranch returns without checkingerr.Suggested fix
- if tc.expectedError { - return - } else { - assert.NoError(t, err) - } + if tc.expectedError { + assert.Error(t, err) + return + } + assert.NoError(t, err)🤖 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 `@db/pkg/db/model/machinecapability_test.go` around lines 1075 - 1085, The negative test case sets expectedError=true but the test harness returns early without asserting that err is non-nil; update the test loop in machinecapability_test.go (the block that inspects case.expectedError) to assert the error properly (e.g., require.Error(t, err) or if err == nil { t.Fatalf("expected error but got nil") }) instead of just returning, so the "Update to invalid Capability Type" case fails when no error is produced; reference the expectedError variable and the local err value in your assertion.
♻️ Duplicate comments (1)
workflow/pkg/activity/machine/machine.go (1)
792-799:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid coercing unknown or absent device types to an empty enum pointer.
Lines 794-795 and 868-869 default
deviceTypeto&"", so nil/unsupported proto values are silently persisted as empty enum values. KeepdeviceTypenil unless a known enum is mapped, and add explicit default handling.Proposed fix
- var deviceType *cdbm.MachineCapabilityDeviceType - dtEmpty := cdbm.MachineCapabilityDeviceType("") - deviceType = &dtEmpty - if gpuCap.DeviceType != nil && *gpuCap.DeviceType == cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_NVLINK { - dt := cdbm.MachineCapabilityDeviceTypeNVLink - deviceType = &dt - } + var deviceType *cdbm.MachineCapabilityDeviceType + if gpuCap.DeviceType != nil { + switch *gpuCap.DeviceType { + case cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_NVLINK: + dt := cdbm.MachineCapabilityDeviceTypeNVLink + deviceType = &dt + default: + slogger.Warn().Int32("gpu_device_type", int32(*gpuCap.DeviceType)).Msg("unsupported GPU device type from site") + } + } @@ - var deviceType *cdbm.MachineCapabilityDeviceType - dtEmpty := cdbm.MachineCapabilityDeviceType("") - deviceType = &dtEmpty - if netCap.DeviceType != nil && *netCap.DeviceType == cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_DPU { - dt := cdbm.MachineCapabilityDeviceTypeDPU - deviceType = &dt - } + var deviceType *cdbm.MachineCapabilityDeviceType + if netCap.DeviceType != nil { + switch *netCap.DeviceType { + case cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_DPU: + dt := cdbm.MachineCapabilityDeviceTypeDPU + deviceType = &dt + default: + slogger.Warn().Int32("network_device_type", int32(*netCap.DeviceType)).Msg("unsupported Network device type from site") + } + }Also applies to: 867-873
🤖 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 `@workflow/pkg/activity/machine/machine.go` around lines 792 - 799, The code currently initializes deviceType to a pointer to an empty enum which causes unknown or absent proto values to be persisted as an empty enum; change the logic in the block that builds deviceType (the variable deviceType, the gpuCap.DeviceType check, and the mapping to cdbm.MachineCapabilityDeviceTypeNVLink) so deviceType remains nil by default and is only set to a non-nil pointer when a known mapping exists (e.g., set deviceType = &dt only when *gpuCap.DeviceType equals cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_NVLINK); remove any dtEmpty := cdbm.MachineCapabilityDeviceType("") / deviceType = &dtEmpty usage and add explicit handling for the unsupported/nil case (leave nil or handle via explicit default branch) including the other similar block around the 867-873 region.
🧹 Nitpick comments (1)
api/pkg/api/handler/instancetype_test.go (1)
721-721: 💤 Low valueConsider nil for truly optional DeviceType fields.
The test fixtures use
cdb.Ptr(cdbm.MachineCapabilityDeviceType(""))to represent an empty device type. If theDeviceTypefield is optional, usingnilwould be more idiomatic and clearer than a pointer to an empty-string enum value. However, if this pattern intentionally tests "present but empty" vs "absent" semantics, retain as is.♻️ Suggested alternative if DeviceType is truly optional
- common.TestBuildMachineCapability(t, dbSession, nil, &it.ID, cdbm.MachineCapabilityTypeCPU, "Intel Xeon E5-2650v2", cdb.GetStrPtr("3.0Hz"), nil, nil, cdb.GetIntPtr(2), cdb.Ptr(cdbm.MachineCapabilityDeviceType("")), nil) + common.TestBuildMachineCapability(t, dbSession, nil, &it.ID, cdbm.MachineCapabilityTypeCPU, "Intel Xeon E5-2650v2", cdb.GetStrPtr("3.0Hz"), nil, nil, cdb.GetIntPtr(2), nil, nil)Also applies to: 784-784, 807-807, 819-819
🤖 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/handler/instancetype_test.go` at line 721, The test fixture is passing an empty-string enum pointer for DeviceType (cdb.Ptr(cdbm.MachineCapabilityDeviceType(""))) in calls to TestBuildMachineCapability; if DeviceType is truly optional, replace those arguments with nil to represent absence (update all occurrences in TestBuildMachineCapability calls at the shown spots), leaving the rest of the call intact so the test expresses "absent" rather than "present-but-empty".
🤖 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.
Outside diff comments:
In `@db/pkg/db/model/machinecapability_test.go`:
- Around line 1075-1085: The negative test case sets expectedError=true but the
test harness returns early without asserting that err is non-nil; update the
test loop in machinecapability_test.go (the block that inspects
case.expectedError) to assert the error properly (e.g., require.Error(t, err) or
if err == nil { t.Fatalf("expected error but got nil") }) instead of just
returning, so the "Update to invalid Capability Type" case fails when no error
is produced; reference the expectedError variable and the local err value in
your assertion.
---
Duplicate comments:
In `@workflow/pkg/activity/machine/machine.go`:
- Around line 792-799: The code currently initializes deviceType to a pointer to
an empty enum which causes unknown or absent proto values to be persisted as an
empty enum; change the logic in the block that builds deviceType (the variable
deviceType, the gpuCap.DeviceType check, and the mapping to
cdbm.MachineCapabilityDeviceTypeNVLink) so deviceType remains nil by default and
is only set to a non-nil pointer when a known mapping exists (e.g., set
deviceType = &dt only when *gpuCap.DeviceType equals
cwssaws.MachineCapabilityDeviceType_MACHINE_CAPABILITY_DEVICE_TYPE_NVLINK);
remove any dtEmpty := cdbm.MachineCapabilityDeviceType("") / deviceType =
&dtEmpty usage and add explicit handling for the unsupported/nil case (leave nil
or handle via explicit default branch) including the other similar block around
the 867-873 region.
---
Nitpick comments:
In `@api/pkg/api/handler/instancetype_test.go`:
- Line 721: The test fixture is passing an empty-string enum pointer for
DeviceType (cdb.Ptr(cdbm.MachineCapabilityDeviceType(""))) in calls to
TestBuildMachineCapability; if DeviceType is truly optional, replace those
arguments with nil to represent absence (update all occurrences in
TestBuildMachineCapability calls at the shown spots), leaving the rest of the
call intact so the test expresses "absent" rather than "present-but-empty".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f36359c7-16d7-4608-8af4-80d27de85a8e
📒 Files selected for processing (28)
AGENTS.mdapi/pkg/api/handler/instance.goapi/pkg/api/handler/instance_test.goapi/pkg/api/handler/instancebatch.goapi/pkg/api/handler/instancebatch_test.goapi/pkg/api/handler/instancetype.goapi/pkg/api/handler/instancetype_test.goapi/pkg/api/handler/machine.goapi/pkg/api/handler/machine_test.goapi/pkg/api/handler/machinecapability_test.goapi/pkg/api/handler/stats.goapi/pkg/api/handler/stats_test.goapi/pkg/api/handler/util/common/common.goapi/pkg/api/handler/util/common/common_test.goapi/pkg/api/handler/util/common/testing.goapi/pkg/api/model/instancetype_test.goapi/pkg/api/model/machinecapability.gocommon/pkg/util/converter.gocommon/pkg/util/converter_test.godb/pkg/db/model/machine_test.godb/pkg/db/model/machinecapability.godb/pkg/db/model/machinecapability_test.godb/pkg/db/util.goworkflow/pkg/activity/instancetype/instancetype.goworkflow/pkg/activity/instancetype/instancetype_test.goworkflow/pkg/activity/machine/machine.goworkflow/pkg/activity/machine/machine_test.goworkflow/pkg/util/conv.go
💤 Files with no reviewable changes (1)
- workflow/pkg/util/conv.go
✅ Files skipped from review due to trivial changes (2)
- AGENTS.md
- api/pkg/api/handler/util/common/common_test.go
chet
force-pushed
the
proto-machinecapability-receivers
branch
2 times, most recently
from
b57e602 to
6d37e38
Compare
Brings `MachineCapability` in line with the proto-conversion rollout. There's no REST endpoint here; this lives entirely in the workflow activity that syncs capability data from Site Agent into the cloud DB. Primary callouts are: - `MachineCapabilityType` and `MachineCapabilityDeviceType` are now typed strings with their own `.ToProto()` and `.FromProto(...)` methods. - `MachineCapability.ToProto` / `FromProto` collapse to one-liner calls for the aforementioned types (exciting). - There is now a separate `MachineCapability.Validate()` that callers run alongside `.FromProto(..)` -- similar to how we `.Validate()` before going `.ToProto()`. - `UpdateInstanceTypeInCloud` becomes a `FromProto` + `Validate` pair, and `AddInstanceTypeToCloud` now uses `MachineCapabilityType.FromProto` for its DB enum conversion As part of the new `MachineCapabilityType` and `MachineCapabilityDeviceType`, I did make some subsequent changes to ensure the typing was better handled through the flow (and not me just type-casting everything as `string`). While this did affect the API layer, nothing is changed on the wire -- JSON serialization/deserialization of type X string is identical to string, OpenAPI spec still just says "string", and the SDK is unaffected. Updated `AGENTS.md` with some notes about named types owning their own `ToProto/FromProto` behavior (like I did here for `MachineCapabilityType` and `MachineCapabilityDeviceType`). Tests added! Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
proto-machinecapability-receivers
branch
from
6d37e38 to
2befdf1
Compare
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
1 participant
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
Brings
MachineCapabilityin line with the proto-modeling changes.Primary callouts are:
MachineCapabilityTypeandMachineCapabilityDeviceTypeare now typed strings with their own.ToProto()and.FromProto(...)methods.MachineCapability.ToProto/FromProtocollapse to one-liner calls for the aforementioned types (exciting).MachineCapability.Validate()that callers run alongside.FromProto(..)-- similar to how we.Validate()before going.ToProto().UpdateInstanceTypeInCloudbecomes aFromProto+Validatepair, andAddInstanceTypeToCloudnow usesMachineCapabilityType.FromProtofor its DB enum conversionAs part of the new
MachineCapabilityTypeandMachineCapabilityDeviceType, I did make some subsequent changes to ensure the typing was better handled through the flow (and not me just type-casting everything asstring). While this did affect the API layer, nothing is changed on the wire -- JSON serialization/deserialization of type X string is identical to string, OpenAPI spec still just says "string", and the SDK is unaffected.Updated
AGENTS.mdwith some notes about named types owning their ownToProto/FromProtobehavior (like I did here forMachineCapabilityTypeandMachineCapabilityDeviceType).Tests added!
Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes