billing: to create update delete and list exclude services #790
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Billing API surface for managing “exclude service” settings/entries, exposing CRUD + list operations via gRPC + HTTP and updating the generated OpenAPI spec.
Changes:
- Added
Create/Update/Delete/ListRPCs for exclude service entries inBillingservice with HTTP mappings. - Introduced new protobuf messages for exclude service entry CRUD + list request/response payloads.
- Updated
openapiv2/apidocs.swagger.jsonto document the new/v1/billinggroups/exclude-service-settingsendpoints and schemas.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openapiv2/apidocs.swagger.json | Documents the new exclude-service-settings REST endpoints and adds request/response schema definitions. |
| billing/v1/billing.proto | Adds new Billing RPCs + message types for exclude service entry CRUD/list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // List Exclude Service Entries | ||
| rpc ListExcludeServices(ListExcludeServicesRequest) returns (ListExcludeServicesResponse) { |
There was a problem hiding this comment.
ListExcludeServices / ListExcludeServicesRequest / ListExcludeServicesResponse reads like it lists cloud services, but the API is modeled around ExcludeServiceEntry and the OpenAPI summary also says “List Exclude Service Entries”. Consider renaming the RPC and request/response types to ListExcludeServiceEntries... (or similar) to match the rest of the CRUD naming (Create/Update/DeleteExcludeServiceEntry) and reduce ambiguity for API consumers.
| rpc ListExcludeServices(ListExcludeServicesRequest) returns (ListExcludeServicesResponse) { | |
| rpc ListExcludeServiceEntries(ListExcludeServicesRequest) returns (ListExcludeServicesResponse) { |
| message ExcludeServiceEntry { | ||
| // MSP (Managed Service Provider) ID. | ||
| string msp_id = 1; | ||
| // Cloud vendor (e.g., "aws", "gcp"). | ||
| string vendor = 2; | ||
| // Payer/account ID. | ||
| string payer_id = 3; | ||
| // Cloud service name. | ||
| string service = 4; | ||
| // Product or SKU code. | ||
| string product = 5; | ||
| // List of payer IDs excepted from exclusion. | ||
| repeated string except_payer_id = 6; |
There was a problem hiding this comment.
The newly added messages use snake_case field names (msp_id, payer_id, except_payer_id, error_message), but the surrounding proto file consistently uses lowerCamelCase field names (e.g., billingGroupId, internalId, groupId). For consistency (and to avoid mixing styles within the same API surface), consider renaming the new fields to match the existing convention.
| // Delete Exclude Service Entry | ||
| rpc DeleteExcludeServiceEntry(DeleteExcludeServiceEntryRequest) returns (DeleteExcludeServiceEntryResponse) { | ||
| option (google.api.http) = { | ||
| delete: "/v1/billinggroups/exclude-service-settings" |
There was a problem hiding this comment.
This DELETE endpoint identifies the resource solely via query parameters. Most existing DELETEs in this API use path parameters for the resource identifier (e.g., /v1/billinggroups/child/{internalId}), which makes the target of the delete unambiguous in routing/logging and in generated clients. Consider revising the HTTP mapping to include the entry’s identifying fields in the path (or otherwise make the delete target explicit), instead of relying on optional query params.
| delete: "/v1/billinggroups/exclude-service-settings" | |
| delete: "/v1/billinggroups/exclude-service-settings/{internalId}" |
No description provided.