Add cloudstack_project resource
#167
Conversation
|
I set this up for Cloudstack API 4.20, I see the tests are 4.18 and 4.19. What's the back version compatibility of this project looking for? There was a difference in the API between 4.18 and 4.19+ for this resource at a quick glance. So if there is a plan to add 4.20 Acceptance Tests and depreciate 4.18 I will wait for those results. |
|
Maybe supersedes #152 |
|
@CodeBleu If possible can you kick a test off here? I merged your actions changes from earlier today to hopefully get the acceptance tests actually working |
ianc769
changed the title
ianc769
changed the title
project resource
ianc769
changed the title
project resourcecloudstack_project resource
Fix issue where getProjectByID() would always return "id not found" while getProjectByName() could find the same project. CloudStack projects are only unique within a domain context, so we now include domain ID in lookups. - Modified getProjectByID() to accept optional domain parameter - Updated all calls to include domain when available - Updated test functions accordingly - Updated documentation to clarify domain requirement for project imports
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Copilot <[email protected]>
…e intended scope Co-authored-by: Copilot <[email protected]>
Bumps [github.com/cloudflare/circl](https://github.com/cloudflare/circl) from 1.3.7 to 1.6.1. - [Release notes](https://github.com/cloudflare/circl/releases) - [Commits](cloudflare/circl@v1.3.7...v1.6.1) --- updated-dependencies: - dependency-name: github.com/cloudflare/circl dependency-version: 1.6.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive support for the cloudstack_project resource to the CloudStack Terraform provider, implementing both the resource and data source functionality for managing CloudStack projects. The implementation includes full CRUD operations, import support, and proper error handling.
Key changes:
- Added
cloudstack_projectresource with create, read, update, delete, and import operations - Added
cloudstack_projectdata source for querying existing projects - Enhanced existing firewall resources to support project context
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
cloudstack/resource_cloudstack_project.go |
Core implementation of the project resource with CRUD operations |
cloudstack/resource_cloudstack_project_test.go |
Comprehensive test suite covering all project resource scenarios |
cloudstack/data_source_cloudstack_project.go |
Data source implementation for querying projects with filtering |
cloudstack/data_source_cloudstack_project_test.go |
Test cases for project data source functionality |
cloudstack/provider.go |
Registration of new resource and data source |
cloudstack/resources.go |
Added domain ID lookup support for project operations |
cloudstack/resource_cloudstack_firewall.go |
Added project parameter support |
cloudstack/resource_cloudstack_egress_firewall.go |
Added project parameter support |
website/docs/r/project.html.markdown |
Documentation for the project resource |
website/docs/d/project.html.markdown |
Documentation for the project data source |
website/cloudstack.erb |
Added navigation link for project resource |
go.mod |
Updated CloudStack Go SDK and dependencies |
README.md |
Minor documentation updates |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| "display_text": { | ||
| Type: schema.TypeString, | ||
| Required: true, // Required for API version 4.18 and lower. TODO: Make this optional when support for API versions older than 4.18 is dropped. |
There was a problem hiding this comment.
The TODO comment should be formatted as a proper Go comment starting with TODO: for better tooling support and clarity.
| Required: true, // Required for API version 4.18 and lower. TODO: Make this optional when support for API versions older than 4.18 is dropped. | |
| Required: true, // Required for API version 4.18 and lower. | |
| // TODO: Make this optional when support for API versions older than 4.18 is dropped. |
Copilot uses AI. Check for mistakes.
| // The CloudStack API parameter order differs between versions: | ||
| // - In API 4.18 and lower: displaytext is the first parameter and name is the second | ||
| // - In API 4.19 and higher: name is the first parameter and displaytext is optional | ||
| // The CloudStack Go SDK uses the API 4.18 parameter order |
There was a problem hiding this comment.
[nitpick] This comment block explains the API version differences but is quite verbose. Consider condensing it to focus on the essential information that affects the current implementation.
| // The CloudStack API parameter order differs between versions: | |
| // - In API 4.18 and lower: displaytext is the first parameter and name is the second | |
| // - In API 4.19 and higher: name is the first parameter and displaytext is optional | |
| // The CloudStack Go SDK uses the API 4.18 parameter order | |
| // The CloudStack Go SDK expects parameters in the API 4.18 order: displaytext, name. |
Copilot uses AI. Check for mistakes.
| // Note: The 'name' parameter is only available in API 4.19 and higher | ||
| // If you're using API 4.18 or lower, the SetName method might not work | ||
| // In that case, you might need to update the display_text only |
There was a problem hiding this comment.
This comment suggests conditional behavior based on API versions but the code doesn't implement this logic. Either implement the version checking or remove the misleading comment.
| // Note: The 'name' parameter is only available in API 4.19 and higher | |
| // If you're using API 4.18 or lower, the SetName method might not work | |
| // In that case, you might need to update the display_text only |
Copilot uses AI. Check for mistakes.
| // Note: accountid is not directly supported by the UpdateProject API, | ||
| // but we can use the account parameter instead if accountid has changed | ||
| if d.HasChange("accountid") && !d.HasChange("account") { | ||
| // If accountid has changed but account hasn't, we need to look up the account name | ||
| // This is a placeholder - in a real implementation, you would need to look up | ||
| // the account name from the accountid | ||
| log.Printf("[WARN] Updating accountid is not directly supported by the API. Please use account instead.") |
There was a problem hiding this comment.
This code block acknowledges incomplete functionality with a placeholder comment. Either implement the account name lookup or remove this feature if it's not supported.
| // Note: accountid is not directly supported by the UpdateProject API, | |
| // but we can use the account parameter instead if accountid has changed | |
| if d.HasChange("accountid") && !d.HasChange("account") { | |
| // If accountid has changed but account hasn't, we need to look up the account name | |
| // This is a placeholder - in a real implementation, you would need to look up | |
| // the account name from the accountid | |
| log.Printf("[WARN] Updating accountid is not directly supported by the API. Please use account instead.") | |
| // Note: accountid is not directly supported by the UpdateProject API. | |
| // If accountid has changed but account hasn't, updating the project owner is not supported. | |
| if d.HasChange("accountid") && !d.HasChange("account") { | |
| log.Printf("[WARN] Updating project owner using only accountid is not supported by the API. Please use the account parameter instead.") |
Copilot uses AI. Check for mistakes.
|
@ianc769, could you please resolve the conflict |
| // but we can use the account parameter instead if accountid has changed | ||
| if d.HasChange("accountid") && !d.HasChange("account") { | ||
| // If accountid has changed but account hasn't, we need to look up the account name | ||
| // This is a placeholder - in a real implementation, you would need to look up |
There was a problem hiding this comment.
@ianc769 Can we implement this? The changes should be small enough and this is most likely already being done at other places in the project.
Adding
projectas a terraform managed resource option -> https://cloudstack.apache.org/api/apidocs-4.20/apis/createProject.htmlContributes to #82
Using this code for example: