Repository navigation
OSAC-2861: add private BareMetalInstanceTypes gRPC service - #40
Merged
openshift-merge-bot[bot] merged 2 commits intoAug 3, 2026
Conversation
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (8)
WalkthroughAdded private bare-metal instance type protobuf contracts, a CRUD and signal gRPC server, server registration, a creation CLI command, and unit and integration coverage for lifecycle operations and immutable fields. ChangesBare-metal instance type management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant BareMetalInstanceTypesServer
participant GenericServer
Client->>BareMetalInstanceTypesServer: Send CRUD or signal request
BareMetalInstanceTypesServer->>BareMetalInstanceTypesServer: Validate or merge resource fields
BareMetalInstanceTypesServer->>GenericServer: Delegate operation
GenericServer-->>BareMetalInstanceTypesServer: Return operation result
BareMetalInstanceTypesServer-->>Client: Return response
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 inconclusive)
✅ Passed checks (10 passed)
✨ 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 |
![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: 6
🤖 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
`@fulfillment-service/internal/servers/private_bare_metal_instance_types_server.go`:
- Around line 336-362: Update the immutability validation around existingHw and
mergedHw to reject removal of required hardware messages, including hardware
itself and its cpu and memory sub-messages. Treat a nil merged hardware, CPU, or
memory value as an InvalidArgument when the corresponding existing value is
present, while preserving the current field-change checks for non-nil pairs.
- Around line 301-319: Replace the hand-rolled merge in
applyBareMetalInstanceTypeUpdate with validation and replacement semantics that
support only valid field-mask paths, including parent paths such as spec, while
rejecting unsupported or nested paths with InvalidArgument before
generic.Update. Ensure masked updates copy metadata so the existing
name-immutability check can detect name changes, and handle nil or empty masks
by cloning the update and replacing the full object rather than using
proto.Merge, allowing repeated fields and maps to be removed. Add unit coverage
for masked name changes, nil-mask disk removal, and unsupported mask paths.
- Around line 237-238: In the Create path before the metadata.name-to-ID
assignment, validate that request.GetObject().GetMetadata().GetName() is
non-empty and return the existing InvalidArgument-style error used by other
mandatory checks when it is missing. Only call SetId after this validation,
preserving the current name-as-primary-key behavior for valid requests.
In `@fulfillment-service/it/it_private_bare_metal_instance_types_test.go`:
- Around line 720-759: Rename the test case around the “Allows deletion…” block
to describe only the successful deletion of an unused bare metal instance type,
since it does not create any referencing instance or verify reference
protection. Keep the existing create, delete, and cleanup behavior unchanged.
- Around line 404-457: Update the DeferCleanup for the finalized object in “Can
delete a bare metal instance type” to first remove the test-finalizer through
the supported update path, then call client.Delete so the row is purged. Ensure
cleanup still runs safely after the test’s initial delete and uses the existing
metadata/finalizer update mechanism.
In
`@fulfillment-service/proto/public/osac/public/v1/baremetal_instance_type_type.proto`:
- Around line 40-46: In the public BareMetalInstanceTypeSpec message, explicitly
reserve field number 2 between hardware and description to preserve alignment
with the private contract and prevent future reuse.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 34034688-78d0-4f6d-be98-68cb4ba1e6f2
⛔ Files ignored due to path filters (8)
fulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gw.gois excluded by!**/*.pb.gw.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_grpc.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/servers/private_bare_metal_instance_types_server.gofulfillment-service/internal/servers/private_bare_metal_instance_types_server_test.gofulfillment-service/it/it_private_bare_metal_instance_types_test.gofulfillment-service/proto/private/osac/private/v1/baremetal_instance_type_type.protofulfillment-service/proto/private/osac/private/v1/baremetal_instance_types_service.protofulfillment-service/proto/public/osac/public/v1/baremetal_instance_type_type.proto
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🤖 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
`@fulfillment-service/internal/servers/private_bare_metal_instance_types_server.go`:
- Around line 336-362: Update the immutability validation around existingHw and
mergedHw to reject removal of required hardware messages, including hardware
itself and its cpu and memory sub-messages. Treat a nil merged hardware, CPU, or
memory value as an InvalidArgument when the corresponding existing value is
present, while preserving the current field-change checks for non-nil pairs.
- Around line 301-319: Replace the hand-rolled merge in
applyBareMetalInstanceTypeUpdate with validation and replacement semantics that
support only valid field-mask paths, including parent paths such as spec, while
rejecting unsupported or nested paths with InvalidArgument before
generic.Update. Ensure masked updates copy metadata so the existing
name-immutability check can detect name changes, and handle nil or empty masks
by cloning the update and replacing the full object rather than using
proto.Merge, allowing repeated fields and maps to be removed. Add unit coverage
for masked name changes, nil-mask disk removal, and unsupported mask paths.
- Around line 237-238: In the Create path before the metadata.name-to-ID
assignment, validate that request.GetObject().GetMetadata().GetName() is
non-empty and return the existing InvalidArgument-style error used by other
mandatory checks when it is missing. Only call SetId after this validation,
preserving the current name-as-primary-key behavior for valid requests.
In `@fulfillment-service/it/it_private_bare_metal_instance_types_test.go`:
- Around line 720-759: Rename the test case around the “Allows deletion…” block
to describe only the successful deletion of an unused bare metal instance type,
since it does not create any referencing instance or verify reference
protection. Keep the existing create, delete, and cleanup behavior unchanged.
- Around line 404-457: Update the DeferCleanup for the finalized object in “Can
delete a bare metal instance type” to first remove the test-finalizer through
the supported update path, then call client.Delete so the row is purged. Ensure
cleanup still runs safely after the test’s initial delete and uses the existing
metadata/finalizer update mechanism.
In
`@fulfillment-service/proto/public/osac/public/v1/baremetal_instance_type_type.proto`:
- Around line 40-46: In the public BareMetalInstanceTypeSpec message, explicitly
reserve field number 2 between hardware and description to preserve alignment
with the private contract and prevent future reuse.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 34034688-78d0-4f6d-be98-68cb4ba1e6f2
⛔ Files ignored due to path filters (8)
fulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gw.gois excluded by!**/*.pb.gw.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_grpc.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
fulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/servers/private_bare_metal_instance_types_server.gofulfillment-service/internal/servers/private_bare_metal_instance_types_server_test.gofulfillment-service/it/it_private_bare_metal_instance_types_test.gofulfillment-service/proto/private/osac/private/v1/baremetal_instance_type_type.protofulfillment-service/proto/private/osac/private/v1/baremetal_instance_types_service.protofulfillment-service/proto/public/osac/public/v1/baremetal_instance_type_type.proto
🛑 Comments failed to post (1)
fulfillment-service/proto/public/osac/public/v1/baremetal_instance_type_type.proto (1)
40-46: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reserve field number 2 in the public
BareMetalInstanceTypeSpec.
hardwareuses tag 1 anddescriptionuses tag 3. Tag 2 is skipped because the private message uses it forhost_label_selector. The gap is intentional but unmarked, so a future author can reuse tag 2 for an unrelated field and break the alignment with the private contract. Reserve it explicitly.🛡️ Proposed change
message BareMetalInstanceTypeSpec { + // Reserved for parity with the private API, where tag 2 is host_label_selector. + reserved 2; + // Hardware specifications for this bare metal instance type. BareMetalHardwareSpec hardware = 1 [(buf.validate.field).required = true]; // Human-readable description of the bare metal instance type. string description = 3; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.message BareMetalInstanceTypeSpec { // Reserved for parity with the private API, where tag 2 is host_label_selector. reserved 2; // Hardware specifications for this bare metal instance type. BareMetalHardwareSpec hardware = 1 [(buf.validate.field).required = true]; // Human-readable description of the bare metal instance type. string description = 3; }🤖 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 `@fulfillment-service/proto/public/osac/public/v1/baremetal_instance_type_type.proto` around lines 40 - 46, In the public BareMetalInstanceTypeSpec message, explicitly reserve field number 2 between hardware and description to preserve alignment with the private contract and prevent future reuse.
ajamias
force-pushed
the
feat/OSAC-2861-private-bare-metal-instance-types-service
branch
from
July 31, 2026 17:33
5b54198 to
ed80248
Compare
ajamias
force-pushed
the
feat/OSAC-2861-private-bare-metal-instance-types-service
branch
from
July 31, 2026 17:44
ed80248 to
78c34bf
Compare
ajamias
force-pushed
the
feat/OSAC-2861-private-bare-metal-instance-types-service
branch
from
July 31, 2026 18:05
78c34bf to
9d20f69
Compare
|
@ajamias: This pull request references OSAC-2861 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. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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
`@fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go`:
- Around line 137-186: Extract the BareMetalInstanceSpec construction currently
in run into a focused helper that accepts the parsed command args and resolved
catalogItem, returning the built spec or an error. Move SSH key, user data,
image source, run-strategy validation, external IP attachment, and
fieldutil.ApplyFields handling into this helper, while leaving catalog lookup
and connection/I/O logic in run. Update run to call the helper and propagate its
error.
In
`@fulfillment-service/internal/servers/private_baremetal_instance_types_server_test.go`:
- Around line 59-73: Add negative tests in the “Behaviour” suite around the
existing server setup to call Create with specifications missing hardware, using
zero cores, missing memory, and an empty host_label_selector. Assert each
request is rejected with grpccodes.InvalidArgument, covering the validation path
for every listed invalid field.
- Around line 520-527: The duplicated stringPtr and int32Ptr helpers must be
removed from both
fulfillment-service/internal/servers/private_baremetal_instance_types_server_test.go:520-527
and fulfillment-service/it/it_private_baremetal_instance_types_test.go:774-781.
Replace every use in both test packages with the shared proto.String and
proto.Int32 helpers, adding the required proto imports while preserving the
existing pointer values.
In
`@fulfillment-service/proto/private/osac/private/v1/baremetal_instance_type_type.proto`:
- Around line 140-157: Update the role field in BareMetalNetworkPortSpec to add
a CEL allow-list validation accepting only fabric, management, storage, or
lifecycle, while retaining the existing non-empty constraint.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 43f07527-47c7-4dbf-b2da-d535670c58ba
⛔ Files ignored due to path filters (8)
fulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gw.gois excluded by!**/*.pb.gw.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_grpc.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.gofulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/servers/private_baremetal_instance_types_server.gofulfillment-service/internal/servers/private_baremetal_instance_types_server_test.gofulfillment-service/it/it_private_baremetal_instance_types_test.gofulfillment-service/proto/private/osac/private/v1/baremetal_instance_type_type.protofulfillment-service/proto/private/osac/private/v1/baremetal_instance_types_service.protofulfillment-service/proto/public/osac/public/v1/baremetal_instance_type_type.proto
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 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
`@fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go`:
- Around line 137-186: Extract the BareMetalInstanceSpec construction currently
in run into a focused helper that accepts the parsed command args and resolved
catalogItem, returning the built spec or an error. Move SSH key, user data,
image source, run-strategy validation, external IP attachment, and
fieldutil.ApplyFields handling into this helper, while leaving catalog lookup
and connection/I/O logic in run. Update run to call the helper and propagate its
error.
In
`@fulfillment-service/internal/servers/private_baremetal_instance_types_server_test.go`:
- Around line 59-73: Add negative tests in the “Behaviour” suite around the
existing server setup to call Create with specifications missing hardware, using
zero cores, missing memory, and an empty host_label_selector. Assert each
request is rejected with grpccodes.InvalidArgument, covering the validation path
for every listed invalid field.
- Around line 520-527: The duplicated stringPtr and int32Ptr helpers must be
removed from both
fulfillment-service/internal/servers/private_baremetal_instance_types_server_test.go:520-527
and fulfillment-service/it/it_private_baremetal_instance_types_test.go:774-781.
Replace every use in both test packages with the shared proto.String and
proto.Int32 helpers, adding the required proto imports while preserving the
existing pointer values.
In
`@fulfillment-service/proto/private/osac/private/v1/baremetal_instance_type_type.proto`:
- Around line 140-157: Update the role field in BareMetalNetworkPortSpec to add
a CEL allow-list validation accepting only fabric, management, storage, or
lifecycle, while retaining the existing non-empty constraint.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 43f07527-47c7-4dbf-b2da-d535670c58ba
⛔ Files ignored due to path filters (8)
fulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gw.gois excluded by!**/*.pb.gw.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_grpc.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.gofulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/servers/private_baremetal_instance_types_server.gofulfillment-service/internal/servers/private_baremetal_instance_types_server_test.gofulfillment-service/it/it_private_baremetal_instance_types_test.gofulfillment-service/proto/private/osac/private/v1/baremetal_instance_type_type.protofulfillment-service/proto/private/osac/private/v1/baremetal_instance_types_service.protofulfillment-service/proto/public/osac/public/v1/baremetal_instance_type_type.proto
🛑 Comments failed to post (2)
fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go (1)
137-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract spec-building logic for testability.
runmixes connection setup, catalog lookup, spec construction, run-strategy validation, and reporting in one function. No test file exists for this command in the current changes. Extract the block that buildsBareMetalInstanceSpec_builder(lines 153-186) into a small helper function that takes the parsed args and the resolvedcatalogItem, and returns the built spec or an error. This isolates pure logic from I/O, so it becomes unit-testable without a gRPC connection.♻️ Proposed extraction
+func buildSpec(args runnerArgs, catalogItem *publicv1.BareMetalInstanceCatalogItem) (*publicv1.BareMetalInstanceSpec, error) { + spec := publicv1.BareMetalInstanceSpec_builder{ + CatalogItem: catalogItem.GetId(), + } + if args.sshKey != "" { + sshKey := args.sshKey + spec.SshPublicKey = &sshKey + } + if args.userData != "" { + userData := args.userData + spec.UserData = &userData + } + if args.imageSourceRef != "" { + spec.Image = publicv1.BareMetalInstanceImage_builder{ + SourceType: args.imageSourceType, + SourceRef: args.imageSourceRef, + }.Build() + } + if args.runStrategy != "" { + val, ok := publicv1.BareMetalInstanceRunStrategy_value["BARE_METAL_INSTANCE_RUN_STRATEGY_"+strings.ToUpper(args.runStrategy)] + if !ok { + return nil, fmt.Errorf( + "unknown run strategy %q, valid values are Always and Halted", + args.runStrategy, + ) + } + rs := publicv1.BareMetalInstanceRunStrategy(val) + spec.RunStrategy = &rs + } + spec.AutoExternalIpAttachment = args.externalIPAttachment + return spec.Build(), nil +}🤖 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 `@fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go` around lines 137 - 186, Extract the BareMetalInstanceSpec construction currently in run into a focused helper that accepts the parsed command args and resolved catalogItem, returning the built spec or an error. Move SSH key, user data, image source, run-strategy validation, external IP attachment, and fieldutil.ApplyFields handling into this helper, while leaving catalog lookup and connection/I/O logic in run. Update run to call the helper and propagate its error.fulfillment-service/proto/private/osac/private/v1/baremetal_instance_type_type.proto (1)
140-157: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Restrict
roleto the documented set of values.The message comment lists exactly four valid roles: fabric, management, storage, lifecycle. The
rolefield only enforcesmin_len = 1, so any non-empty string passes validation. A typo (for example,fabrik) reaches storage undetected, and any code that branches on role becomes fragile.Add a CEL rule restricting
roleto the enumerated set, since this is cheaper than a wire-format change to an enum at this stage.🔧 Suggested allow-list constraint
// Port role (e.g. fabric, management, storage, lifecycle). - string role = 2 [(buf.validate.field).string.min_len = 1]; + string role = 2 [(buf.validate.field).cel = { + id: "network_port_spec.role_valid" + message: "role must be one of: fabric, management, storage, lifecycle" + expression: "this in ['fabric', 'management', 'storage', 'lifecycle']" + }];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Describes a network port in a bare metal instance type. // // Port roles: fabric (tenant data traffic), management (control plane), // storage (storage fabric), lifecycle (BMC/PXE, not tenant-attachable). // Ports are ordered; the first port with a given role is the default for that role. message BareMetalNetworkPortSpec { // Port name, unique within the type (e.g. data-0, mgmt-0). string name = 1 [(buf.validate.field).string.min_len = 1]; // Port role (e.g. fabric, management, storage, lifecycle). string role = 2 [(buf.validate.field).cel = { id: "network_port_spec.role_valid" message: "role must be one of: fabric, management, storage, lifecycle" expression: "this in ['fabric', 'management', 'storage', 'lifecycle']" }]; // Port type (e.g. Ethernet, InfiniBand). string type = 3 [(buf.validate.field).string.min_len = 1]; // Port speed (e.g. 1Gbps, 10Gbps, 100Gbps). string speed = 4 [(buf.validate.field).string.min_len = 1]; }🤖 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 `@fulfillment-service/proto/private/osac/private/v1/baremetal_instance_type_type.proto` around lines 140 - 157, Update the role field in BareMetalNetworkPortSpec to add a CEL allow-list validation accepting only fabric, management, storage, or lifecycle, while retaining the existing non-empty constraint.
ajamias
force-pushed
the
feat/OSAC-2861-private-bare-metal-instance-types-service
branch
from
July 31, 2026 20:35
9d20f69 to
4890e5f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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
`@fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go`:
- Around line 131-135: Update the deferred cleanup after cfg.Connect in the
create command to call conn.Close through a deferred function and log any
returned error with the existing slog logger, preserving the current connection
cleanup while preventing close failures from being discarded.
- Around line 100-102: Validate the catalog-item flag value immediately after
the required-flag setup in the bare-metal instance command, before configuration
initialization or lookup.Find. Reject an empty value with the user-facing error
“catalog item must not be empty,” while preserving the existing required-flag
and lookup behavior for non-empty values.
In
`@fulfillment-service/internal/servers/private_baremetal_instance_types_server_test.go`:
- Around line 202-250: Extend the `Update object` specs to cover the
`spec.host_label_selector` update-mask path in
`applyBareMetalInstanceTypeUpdate`, verifying the selector changes while
unrelated fields remain intact. Add a separate `Update` request with an empty
object ID and assert it returns an `InvalidArgument` error, covering the guard
in the server’s `Update` handler.
In `@fulfillment-service/it/it_private_baremetal_instance_types_test.go`:
- Around line 720-759: Rename the test beginning with “Can delete an unused bare
metal instance type” and update its description/comment to describe only
successful deletion of an unreferenced type, removing the “deletion protection”
wording. Keep the existing create, delete, and cleanup flow unchanged.
- Around line 501-503: Add negative Create-request specs to the Error scenarios
section of the integration suite, covering missing hardware, zero cores, missing
memory, and an empty host_label_selector. Send each request through the complete
gRPC Create path and assert that every response returns
grpccodes.InvalidArgument, preserving the existing immutability and not-found
scenarios.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa2d9aac-5efd-4ac0-997a-f5d148d94fba
📒 Files selected for processing (5)
fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.gofulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/servers/private_baremetal_instance_types_server.gofulfillment-service/internal/servers/private_baremetal_instance_types_server_test.gofulfillment-service/it/it_private_baremetal_instance_types_test.go
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🤖 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
`@fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go`:
- Around line 131-135: Update the deferred cleanup after cfg.Connect in the
create command to call conn.Close through a deferred function and log any
returned error with the existing slog logger, preserving the current connection
cleanup while preventing close failures from being discarded.
- Around line 100-102: Validate the catalog-item flag value immediately after
the required-flag setup in the bare-metal instance command, before configuration
initialization or lookup.Find. Reject an empty value with the user-facing error
“catalog item must not be empty,” while preserving the existing required-flag
and lookup behavior for non-empty values.
In
`@fulfillment-service/internal/servers/private_baremetal_instance_types_server_test.go`:
- Around line 202-250: Extend the `Update object` specs to cover the
`spec.host_label_selector` update-mask path in
`applyBareMetalInstanceTypeUpdate`, verifying the selector changes while
unrelated fields remain intact. Add a separate `Update` request with an empty
object ID and assert it returns an `InvalidArgument` error, covering the guard
in the server’s `Update` handler.
In `@fulfillment-service/it/it_private_baremetal_instance_types_test.go`:
- Around line 720-759: Rename the test beginning with “Can delete an unused bare
metal instance type” and update its description/comment to describe only
successful deletion of an unreferenced type, removing the “deletion protection”
wording. Keep the existing create, delete, and cleanup flow unchanged.
- Around line 501-503: Add negative Create-request specs to the Error scenarios
section of the integration suite, covering missing hardware, zero cores, missing
memory, and an empty host_label_selector. Send each request through the complete
gRPC Create path and assert that every response returns
grpccodes.InvalidArgument, preserving the existing immutability and not-found
scenarios.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fa2d9aac-5efd-4ac0-997a-f5d148d94fba
📒 Files selected for processing (5)
fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.gofulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/servers/private_baremetal_instance_types_server.gofulfillment-service/internal/servers/private_baremetal_instance_types_server_test.gofulfillment-service/it/it_private_baremetal_instance_types_test.go
🛑 Comments failed to post (2)
fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go (2)
100-102: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In github.com/spf13/cobra v1.10.2, does Command.MarkFlagRequired consider a string flag supplied as --catalog-item="" valid because the flag was changed?💡 Result:
Yes, in spf13/cobra v1.10.2, Command.MarkFlagRequired considers the flag --catalog-item="" to be valid (i.e., not missing) [1][2][3]. Cobra's
MarkFlagRequiredmechanism validates only the presence of the flag on the command line, not the content of its value [3]. When you provide --catalog-item="", the flag is considered "set" (changed) by the underlying pflag library, which satisfies Cobra's requirement check [4][3]. The validation logic in Cobra, specifically within theValidateRequiredFlagsmethod, confirms this behavior [4]: 1. It checks if the flag has theBashCompOneRequiredFlagannotation (applied byMarkFlagRequired) [5][4]. 2. If the annotation is present, it verifies whether the flag has beenChanged[4]. 3. Because providing the flag—even with an empty string—marks it asChangedin thepflag.FlagSet, Cobra considers the requirement satisfied and does not return an error [4][3]. If you need to ensure the value is non-empty, you must implement custom validation logic in your code (e.g., inPreRunEor a custompflag.Valueimplementation) to check the flag's value after it has been parsed [1][3][6].Citations:
- 1: spf13/cobra#1898
- 2: spf13/cobra#747
- 3: spf13/cobra#909
- 4: https://github.com/spf13/cobra/blob/v1.10.2/command.go
- 5: https://github.com/spf13/cobra/blob/v1.10.2/shell_completions.go
- 6: https://cobra.dev/docs/how-to-guides/working-with-flags/
🏁 Script executed:
#!/bin/bash set -euo pipefail file="fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go" if [ ! -f "$file" ]; then echo "File not found: $file" fd -a 'create_baremetal_instance_cmd.go' . exit 1 fi echo "== file outline ==" ast-grep outline "$file" || true echo "== relevant lines 1-180 ==" sed -n '1,180p' "$file" | nl -ba echo "== lookup usage / package references ==" rg -n "catalogItem|catalog-item|Find|MarkFlagRequired|cobra.Command|pflag" "$file" .Repository: osac-project/osac
Length of output: 815
🏁 Script executed:
#!/bin/bash set -euo pipefail file="fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go" echo "== file exists and line count ==" [ -f "$file" ] || { echo "file missing"; exit 1; } wc -l "$file" echo "== lines 1-180 ==" awk '{printf "%5d\t%s\n", NR, $0}' "$file" | sed -n '1,180p' echo "== searches in file ==" rg -n "catalogItem|catalog-item|Find|MarkFlagRequired|cobra.Command|pflag" "$file" || true echo "== find lookup identifiers ==" rg -n "type .*Find|func .*Find|package lookup|Find\\(" fulfillment-service/internal -S || trueRepository: osac-project/osac
Length of output: 15000
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== lookup.go ==" sed -n '1,140p' fulfillment-service/internal/cmd/cli/lookup/lookup.go | awk '{printf "%5d\t%s\n", NR, $0}' echo "== lookup tests around empty ref ==" sed -n '90,135p' fulfillment-service/internal/cmd/cli/lookup/lookup_test.go | awk '{printf "%5d\t%s\n", NR, $0}'Repository: osac-project/osac
Length of output: 4628
Reject empty catalog item values before lookup.
--catalog-item=""satisfies Cobra’s flag requirement but reacheslookup.Findwith an empty reference, so the command currently reports a generic catalog item lookup error. Return a user-facing “catalog item must not be empty” error here before configuration setup or network calls.Proposed fix
func (c *runnerContext) run(cmd *cobra.Command, _ []string) error { ctx := cmd.Context() c.logger = logging.LoggerFromContext(ctx) console := terminal.ConsoleFromContext(ctx) + if strings.TrimSpace(c.args.catalogItem) == "" { + return fmt.Errorf("catalog item must not be empty") + } + cfg := config.SettingsFromContext(ctx)🤖 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 `@fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go` around lines 100 - 102, Validate the catalog-item flag value immediately after the required-flag setup in the bare-metal instance command, before configuration initialization or lookup.Find. Reject an empty value with the user-facing error “catalog item must not be empty,” while preserving the existing required-flag and lookup behavior for non-empty values.
131-135: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n -C 5 'func\s+\([^)]*\)\s+Connect\(' fulfillment-service/internal/config rg -n -C 3 'defer\s+conn\.Close\(\)' fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.goRepository: osac-project/osac
Length of output: 1557
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== file size ==" wc -l fulfillment-service/internal/config/config_settings.go fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go echo "== Connect implementation ==" sed -n '482,530p' fulfillment-service/internal/config/config_settings.go echo "== logger usages nearby command ==" sed -n '1,180p' fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go echo "== conn.Close error handling in Go files (patterns) ==" rg -n 'Close\(\) \)\s*\{[^\n]*return[^\n]*Close\(\)' --glob '*.go' . || true rg -n -C 2 'defer\s+conn\.Close\(\)|Close\(\) != nil|CloseError|closeErr' --glob '*.go' . | head -n 120Repository: osac-project/osac
Length of output: 19339
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== grpc ClientConn Close signature ==" if [ -d vendor ] && [ -f vendor/google.golang.org/grpc/clientconn.go ]; then rg -n -C 3 'func \(.+\)\s+Close\s*\(' vendor/google.golang.org/grpc/clientconn.go elif command -v go >/dev/null 2>&1; then tmp="$(mktemp -d)" cd "$tmp" >/dev/null module="$(pwd)/module" mkdir -p "$module/google.golang.org/grpc/internal/transport" cat > go.mod <<'MOD' module module go 1.22 require google.golang.org/grpc v1.74.2 MOD go mod download google.golang.org/grpc > /dev/null 2>&1 || true rg -n -C 3 'func \(.+\)\s+Close\s*\(' "$(go env GOMODCACHE)/google.golang.org/grpc@*/clientconn.go" 2>/dev/null | head -n 80 else echo "unable to inspect grpc ClientConn Close signature locally" fi echo "== runnerContext fields and assignments in command ==" rg -n -C 2 'type runnerContext struct|logger\*slog.Logger|logging.LoggerFromContext|ConsoleFromContext' fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.goRepository: osac-project/osac
Length of output: 193
Handle the gRPC connection close error.
conn.Close()can return an error, but line 135 discards it. Close the deferred connection through a function that logs any close failure so resource-cleanup failures fromslogare not lost.Proposed fix
- defer conn.Close() + defer func() { + if closeErr := conn.Close(); closeErr != nil { + c.logger.Error("failed to close gRPC connection", "error", closeErr) + Slog + }()🤖 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 `@fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go` around lines 131 - 135, Update the deferred cleanup after cfg.Connect in the create command to call conn.Close through a deferred function and log any returned error with the existing slog logger, preserving the current connection cleanup while preventing close failures from being discarded.Source: Path instructions
eliorerz
pushed a commit
that referenced
this pull request
Aug 1, 2026
…-deprovision feat: Rename ComputeInstance env vars to PROVISION/DEPROVISION terminology
ajamias
force-pushed
the
feat/OSAC-2861-private-bare-metal-instance-types-service
branch
from
August 3, 2026 14:07
4890e5f to
4964eed
Compare
Add private and public BareMetalInstanceTypes proto contracts with gRPC service definitions, buf.validate annotations, and REST transcoding. Reserve field 2 in public BareMetalInstanceTypeSpec for parity with the private contract's host_label_selector field. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Austin Jamias <ajamias@redhat.com>
Add PrivateBareMetalInstanceTypesServer with CRUD operations, field-mask update support, and hardware immutability enforcement using proto.Equal. Register the server in the gRPC startup. Field validation is handled by the protovalidate interceptor via buf.validate annotations. Include unit tests (builder, CRUD, immutability) and integration tests (lifecycle, field-mask updates, deletion protection, immutability). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Austin Jamias <ajamias@redhat.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go`:
- Around line 131-135: Update the deferred cleanup around cfg.Connect in the
command execution flow to capture the error returned by conn.Close and return it
only when the surrounding operation has not already produced an error. Preserve
the existing connection error wrapping and ensure earlier errors take precedence
over the close error.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69ca8a20-1748-455f-8a00-54af8b933f95
⛔ Files ignored due to path filters (8)
fulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gw.gois excluded by!**/*.pb.gw.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_grpc.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.gofulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/servers/private_baremetal_instance_types_server.gofulfillment-service/internal/servers/private_baremetal_instance_types_server_test.gofulfillment-service/it/it_private_baremetal_instance_types_test.gofulfillment-service/proto/private/osac/private/v1/baremetal_instance_type_type.protofulfillment-service/proto/private/osac/private/v1/baremetal_instance_types_service.protofulfillment-service/proto/public/osac/public/v1/baremetal_instance_type_type.proto
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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
`@fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go`:
- Around line 131-135: Update the deferred cleanup around cfg.Connect in the
command execution flow to capture the error returned by conn.Close and return it
only when the surrounding operation has not already produced an error. Preserve
the existing connection error wrapping and ensure earlier errors take precedence
over the close error.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69ca8a20-1748-455f-8a00-54af8b933f95
⛔ Files ignored due to path filters (8)
fulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service.pb.gw.gois excluded by!**/*.pb.gw.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_grpc.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/private/v1/baremetal_instance_types_service_protoopaque.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type.pb.gois excluded by!**/*.pb.gofulfillment-service/internal/api/osac/public/v1/baremetal_instance_type_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.gofulfillment-service/internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gofulfillment-service/internal/servers/private_baremetal_instance_types_server.gofulfillment-service/internal/servers/private_baremetal_instance_types_server_test.gofulfillment-service/it/it_private_baremetal_instance_types_test.gofulfillment-service/proto/private/osac/private/v1/baremetal_instance_type_type.protofulfillment-service/proto/private/osac/private/v1/baremetal_instance_types_service.protofulfillment-service/proto/public/osac/public/v1/baremetal_instance_type_type.proto
🛑 Comments failed to post (1)
fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go (1)
131-135: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Confirm the concrete type returned by config.Settings.Connect and its Close signature. rg -n -C 5 --glob '*.go' \ 'func \(.*Settings.*\) Connect\(|func .*Connect\(' \ fulfillment-service/internal/config || trueRepository: osac-project/osac
Length of output: 1257
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== candidate file outline ==" ast-grep outline fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go || true echo echo "== relevant command excerpt ==" sed -n '100,155p' fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go echo echo "== Close usages in file ==" rg -n -C 3 'defer\s+conn\.Close\(\)' fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go || true echo echo "== grpc ClientConn Close signature in module vendor/cache? ==" # Read-only search of Go module cache if available; do not install dependencies or run repo code. find "${GOPATH:-$HOME/go}/pkg/mod" -path '*/google.golang.org/grpc@*' -name 'clientconn.go' 2>/dev/null | head -5 | while read -r f; do echo "-- $f" rg -n -C 3 'func .*ClientConn.*Close\s*\(' "$f" doneRepository: osac-project/osac
Length of output: 3111
Handle
conn.Close()errors.
grpc.ClientConn.Close()returns an error, so thisdefer conn.Close()violates the repository’s “Never ignore error returns” rule. Return the close error only when no earlier error occurred.🤖 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 `@fulfillment-service/internal/cmd/cli/create/baremetalinstance/create_baremetal_instance_cmd.go` around lines 131 - 135, Update the deferred cleanup around cfg.Connect in the command execution flow to capture the error returned by conn.Close and return it only when the surrounding operation has not already produced an error. Preserve the existing connection error wrapping and ensure earlier errors take precedence over the close error.Source: Path instructions
adriengentil
approved these changes
Aug 3, 2026
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, ajamias 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 |
ajamias
deleted the
feat/OSAC-2861-private-bare-metal-instance-types-service
branch
August 3, 2026 16:05
This was referenced Aug 3, 2026
eliorerz
pushed a commit
that referenced
this pull request
Sep 16, 2026
OSAC-1560: add Metal3 backend configuration guide
This branch was previously deployed
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
3 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.
OSAC-2861: add private BareMetalInstanceTypes gRPC service
Summary
Implements the private gRPC service for managing bare metal instance types, providing full CRUD operations plus Signal RPC. This service follows the GenericServer delegation pattern used by all other private servers, enabling operators to define hardware profiles that match physical bare metal hosts to workload requirements.
Changes
baremetal_instance_types_service.protowith List/Get/Create/Update/Delete/Signal RPCs and REST transcoding annotationsPrivateBareMetalInstanceTypesServerwith builder pattern, input validation for hardware specs (CPU, memory, disks, accelerators, network ports), host label selector validation, field mask updates, and immutability enforcement for core hardware fieldsbuf generateTesting
Dependencies
bare_metal_instance_typestable) — already mergedAssisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
New Features
Validation