-
Notifications
You must be signed in to change notification settings - Fork 47
Add cloudstack_limits
data source and resource
#197
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
base: main
Are you sure you want to change the base?
Conversation
- Implemented data source for retrieving CloudStack resource limits. - Added resource management for setting and updating resource limits for accounts, domains, and projects. - Updated documentation for cloudstack_limits with usage examples and argument references.
…ted resources Implimenting copilot suggestions for d.GetOk
Co-authored-by: Copilot <[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
This PR adds CloudStack limits as both a Terraform data source and resource, enabling management of resource limits for accounts, domains, and projects within CloudStack through the updateResourceLimit API.
- Adds cloudstack_limits resource for managing resource limits with support for various resource types (instance, ip, volume, snapshot, template, project, network, vpc, cpu, memory, primarystorage, secondarystorage)
- Adds cloudstack_limits data source for retrieving existing resource limit information
- Provides comprehensive test coverage for different resource types and scenarios including unlimited and zero limits
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
cloudstack/provider.go | Registers the new data source and resource in the provider |
cloudstack/resource_cloudstack_limits.go | Core resource implementation with CRUD operations for managing CloudStack limits |
cloudstack/data_source_cloudstack_limits.go | Data source implementation for reading CloudStack resource limits |
cloudstack/resource_cloudstack_limits_test.go | Comprehensive test suite covering various limit scenarios and resource types |
website/docs/r/limits.html.markdown | Resource documentation with usage examples and parameter descriptions |
website/docs/d/limits.html.markdown | Data source documentation with usage examples and attribute descriptions |
resource "cloudstack_limits" "project_limit" { | ||
type = "primarystorage" | ||
max = 1000 | ||
domainid = cloudstack_domain.test_domain.id |
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 test configuration testAccCloudStackLimits_project
is missing the actual project resource creation. It only sets a domain-level limit instead of a project-specific limit. This doesn't match the test name and will cause the test to fail since no project is created for the projectid
parameter.
domainid = cloudstack_domain.test_domain.id | |
resource "cloudstack_project" "test_project" { | |
name = "test-project-limits" | |
display_text = "Test Project for Limits" | |
domainid = cloudstack_domain.test_domain.id | |
} | |
resource "cloudstack_limits" "project_limit" { | |
type = "primarystorage" | |
max = 1000 | |
projectid = cloudstack_project.test_project.id | |
domainid = cloudstack_domain.test_domain.id |
Copilot uses AI. Check for mistakes.
// If the user set max to 0 but the API returned -1, keep it as 0 in the state | ||
if limit.Max == -1 && d.Get("max").(int) == 0 { | ||
log.Printf("[DEBUG] API returned -1 for a limit set to 0, keeping it as 0 in state") | ||
d.Set("max", 0) |
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.
This condition assumes that d.Get("max")
will always return an int, but if the max field is not set in the schema, this could cause a panic. Consider checking if the value exists first with d.GetOk("max")
before type assertion.
d.Set("max", 0) | |
if limit.Max == -1 { | |
if v, ok := d.GetOk("max"); ok && v.(int) == 0 { | |
log.Printf("[DEBUG] API returned -1 for a limit set to 0, keeping it as 0 in state") | |
d.Set("max", 0) | |
} else { | |
d.Set("max", limit.Max) | |
} |
Copilot uses AI. Check for mistakes.
}, | ||
} | ||
} | ||
|
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 import functionality uses ImportStatePassthroughContext
but the documentation shows specific import ID formats like 2-acct1-domain-uuid
. Consider implementing a custom import function that properly parses these composite IDs or update the documentation to reflect the actual import behavior.
StateContext: resourceCloudStackLimitsImport, | |
}, | |
} | |
} | |
// resourceCloudStackLimitsImport parses composite import IDs and sets resource fields accordingly. | |
func resourceCloudStackLimitsImport(ctx context.Context, d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { | |
// Expected format: type-account-domainid or type-projectid | |
// Example: 2-acct1-domain-uuid or 2-projectid | |
parts := strings.Split(d.Id(), "-") | |
if len(parts) < 2 { | |
return nil, fmt.Errorf("unexpected import ID format (%q), expected type-account-domainid or type-projectid", d.Id()) | |
} | |
// Set type | |
typeInt, err := strconv.Atoi(parts[0]) | |
if err != nil { | |
return nil, fmt.Errorf("invalid type value in import ID: %s", parts[0]) | |
} | |
var typeStr string | |
for k, v := range resourceTypeMap { | |
if v == typeInt { | |
typeStr = k | |
break | |
} | |
} | |
if typeStr == "" { | |
return nil, fmt.Errorf("unknown type value in import ID: %d", typeInt) | |
} | |
if err := d.Set("type", typeStr); err != nil { | |
return nil, err | |
} | |
// Set other fields | |
if len(parts) == 3 { | |
// type-account-domainid | |
if err := d.Set("account", parts[1]); err != nil { | |
return nil, err | |
} | |
if err := d.Set("domainid", parts[2]); err != nil { | |
return nil, err | |
} | |
} else if len(parts) == 2 { | |
// type-projectid | |
if err := d.Set("projectid", parts[1]); err != nil { | |
return nil, err | |
} | |
} else { | |
return nil, fmt.Errorf("unexpected import ID format (%q), expected type-account-domainid or type-projectid", d.Id()) | |
} | |
return []*schema.ResourceData{d}, nil | |
} |
Copilot uses AI. Check for mistakes.
Adding limits as a terraform managed resource option -> https://cloudstack.apache.org/api/apidocs-4.20/apis/updateResourceLimit.html
Contributes to #82
Using this code for example: