-
Notifications
You must be signed in to change notification settings - Fork 36
Fix format of details map for update VM #118
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
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.
Code looks good, any chance testing needs updating or can be added; it seems a bit of a hack and would benefit from saveguarding it.
I can add a basic test in |
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
This pull request fixes the format of details map for update VM operations by improving parameter indexing logic in the CloudStack Terraform provider code generation. The changes ensure that the updateVirtualMachine
command's details
parameter uses zero-based indexing format instead of sequential indexing.
- Added
updateVirtualMachine
to commands requiring zero-based indexing for details parameters - Introduced new parameter mapping to control when index variables are needed in code generation
- Updated code generation logic to conditionally apply zero-based vs sequential indexing based on parameter requirements
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
generate/generate.go | Added configuration maps and updated code generation logic to handle parameter indexing requirements |
cloudstack/VirtualMachineService.go | Updated UpdateVirtualMachineParams.toURLValues method to use zero-based indexing for details parameter |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
clgtm
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.
LGTM, tested with terraform pr
Ref: apache/cloudstack-terraform-provider#158 (review)
This pull request improves the handling and code generation for parameters that require specific indexing formats in CloudStack API requests. The main changes address how map parameters are serialized, especially for commands and parameters that use zero-based indexing or require explicit index variables.
Improvements to parameter indexing logic:
updateVirtualMachine
to thedetailsRequireZeroIndex
map, ensuring itsdetails
parameter uses zero-based indexing in generated code.parametersRequireIndexing
map to track parameters that always need index variables, even if the command uses zero indexing.Adjustments to code generation:
generateConvertCode
to use index variables only when required by the new maps, improving accuracy for map parameter serialization.Bug fix in VM update parameter serialization:
UpdateVirtualMachineParams.toURLValues
method to always use zero-based indexing for thedetails
parameter, aligning with API expectations.