-
Notifications
You must be signed in to change notification settings - Fork 14
Add sync_currency api to report_adjustment_policy.proto for currency #580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…t_policy.proto for improved clarity Signed-off-by: ImMin5 <[email protected]>
…mentUnit for clarity Signed-off-by: ImMin5 <[email protected]>
…rency synchronization Signed-off-by: ImMin5 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a new currency synchronization endpoint and extends/enhances existing proto definitions for adjustment policies and reports.
- Introduce a
sync_currencyRPC under the ReportAdjustmentPolicy service - Extend
Scopeenums everywhere with aSERVICE_ACCOUNTmember - Restructure policy messages (remove
name, adddescription, reorder fields) and renameAdjustmentMethod→AdjustmentUnitin report adjustments
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| proto/spaceone/api/cost_analysis/v1/report_adjustment_policy.proto | Added sync_currency RPC; appended SERVICE_ACCOUNT to Scope enums; reordered and renamed fields in Create/Update/Info messages |
| proto/spaceone/api/cost_analysis/v1/report_adjustment.proto | Renamed AdjustmentMethod enum to AdjustmentUnit and updated all references |
| proto/spaceone/api/cost_analysis/v1/cost_report_config.proto | Appended SERVICE_ACCOUNT to the Scope enum |
Comments suppressed due to low confidence (3)
proto/spaceone/api/cost_analysis/v1/report_adjustment_policy.proto:42
- There are no tests covering the new 'sync_currency' RPC. Add unit or integration tests to verify its behavior and HTTP mapping.
rpc sync_currency (ReportAdjustmentPolicyRequest) returns (ReportAdjustmentPolicyInfo) {
proto/spaceone/api/cost_analysis/v1/report_adjustment_policy.proto:77
- Reordering and reusing field tags (removing 'name' and shifting 'scope' from 2→1) breaks backward compatibility. Instead, introduce new fields for additions and deprecate old ones without renumbering existing tags.
Scope scope = 1;
proto/spaceone/api/cost_analysis/v1/report_adjustment.proto:58
- Renaming the enum from 'AdjustmentMethod' to 'AdjustmentUnit' is a breaking change. Consider deprecating the old enum and introducing the new one alongside it to maintain compatibility.
enum AdjustmentUnit {
| }; | ||
| } | ||
|
|
||
| rpc sync_currency (ReportAdjustmentPolicyRequest) returns (ReportAdjustmentPolicyInfo) { |
Copilot
AI
May 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a generic ReportAdjustmentPolicyRequest for the new sync_currency RPC may obscure which fields are required. Define a dedicated SyncCurrencyRequest message with explicit parameters to improve clarity.
| rpc sync_currency (ReportAdjustmentPolicyRequest) returns (ReportAdjustmentPolicyInfo) { | |
| rpc sync_currency (SyncCurrencyRequest) returns (ReportAdjustmentPolicyInfo) { |
| int32 order = 3; | ||
| // +optional | ||
| google.protobuf.Struct tags = 6; | ||
| string description = 4; |
Copilot
AI
May 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'description' field is intended to be optional but lacks a '+optional' annotation. Add '+optional' to align with other fields and improve readability.
Category
Description
Known issue