service offering. sdk framework rewrite#138
Conversation
|
@poddm Can you add the updated docs to this PR as well? I believe you had docs on the other PR you closed, but there doesn't appear to be updated docs for this. I have pulled down your branch to test, and am still having issues with needing to specify the I get the following error: I'm using it like this: my locals |
|
@CodeBleu , I fixed the domain ids issue. Try now. Also, regarding the documentation. I was looking at using the generator but it might be better to scope all that work to its own PR. https://github.com/hashicorp/terraform-plugin-docs |
@poddm Thanks!!
I just did a you can see above that the Also, why is storage_tags avail in both at the 'root' level of the resource and also as
|
I think it would be better to add a data resource for domains. The problem I see with this approach is the resource will never become tainted if it was recreated as the same name. Subsequent resources that may modify or depend on the domain would never detect the newly re/created domain. ex. data "cloudstack_domain" "ace" {
name = "/ROOT/ACE"
}
resource "cloudstack_service_offering_fixed" "standard_offering" {
...
domain_ids = [data.cloudstack_domain.ace.id]
}
Let me check. I believe
Thats a bug. storage tags should be in the |
|
@poddm Thank you so much for looking into this. Once this offering stuff is complete, I still need a way to implement this with the Is this something you can fix with a new PR for |
|
@poddm I haven' tested but PR looks good. Left some minor comments. |
You can provide the values in the details field. resource "cloudstack_instance" "this" {
name = cloud
display_name = "cloud details"
details = {cpuNumber = "4", memory = "2048"}
} |
@poddm This is not currently part of the provider. Do you have this in your own branch? If so, Is there a PR for this too? https://registry.terraform.io/providers/cloudstack/cloudstack/latest/docs/resources/instance I tried to use what you show anyway, and I get this: |
|
@poddm Not sure if you saw that I'm still having issues with this or not. Wondering what I need to do? |
…to null, moved storage_tags field to nested block disk_offering
I fixed the duplicate storage_offering field.
This usually means the management server api returned an html error response and not json like the client library is expecting. I've been using this in |
Thanks, I will test this later. My main issue was the
What should I do?
|
|
@poddm Was wondering if you could help me with this ☝️ ? I need the |
fixed. |
@poddm Thanks, I do see that it is showing the
|
|
The error is being thrown from cloudstacks API. Try setting this global var to 0. |
Not really sure where you are talking about. Before the last change, this wasn't an issue and would create the offering with a In my I have this: |
|
@poddm Reviewing this PR again, I believe the only issue remaining ( that I had) was the Update: I went ahead and committed my suggestion for the |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the service offering resources to support the full CloudStack API specification by implementing a complete SDK framework rewrite. The main purpose is to split the service offering API into three distinct resource types based on different Quality of Service (QoS) types, making it clearer for users to understand the different offering configurations.
- Implements three new service offering resource types: constrained, unconstrained, and fixed
- Updates the Terraform plugin framework to version 1.12.0 and related dependencies
- Adds comprehensive test coverage for all three resource types with various disk configurations
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updates Terraform plugin framework and related dependencies to newer versions |
| cloudstack/service_offering_util.go | Implements common utility functions for CRUD operations across all service offering types |
| cloudstack/service_offering_schema.go | Defines shared schema attributes for service offering resources with nested disk configurations |
| cloudstack/service_offering_models.go | Defines Go struct models for the three service offering types and their nested objects |
| cloudstack/service_offering_*_resource.go | Implements the three service offering resource types with full CRUD operations |
| cloudstack/service_offering_*_resource_test.go | Comprehensive test cases for all three resource types |
| cloudstack/provider_v6.go | Registers the new service offering resources with the provider |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| state.DisplayText = types.StringValue(cs.Displaytext) | ||
| } | ||
| if cs.Domainid != "" { | ||
| state.DomainIds, _ = types.SetValueFrom(ctx, types.StringType, strings.Split(cs.Domainid, ",")) |
There was a problem hiding this comment.
Error from types.SetValueFrom is ignored. This could lead to silent failures when converting domain IDs. The error should be handled properly or at least logged.
| state.Name = types.StringValue(cs.Name) | ||
| } | ||
| if cs.Zoneid != "" { | ||
| state.ZoneIds, _ = types.SetValueFrom(ctx, types.StringType, strings.Split(cs.Zoneid, ",")) |
There was a problem hiding this comment.
Error from types.SetValueFrom is ignored. This could lead to silent failures when converting zone IDs. The error should be handled properly or at least logged.
| p.SetDisplaytext(plan.DisplayText.ValueString()) | ||
| } | ||
| if !plan.DomainIds.IsNull() { | ||
| p.SetDomainid(plan.DomainIds.String()) |
There was a problem hiding this comment.
Using plan.DomainIds.String() directly may not provide the correct format for the API. This will likely result in a string representation like '["id1", "id2"]' instead of a comma-separated list. Consider extracting the actual string values and joining them with commas.
| p.SetName(plan.Name.ValueString()) | ||
| } | ||
| if !plan.ZoneIds.IsNull() && len(plan.ZoneIds.Elements()) > 0 { | ||
| p.SetZoneid(plan.ZoneIds.String()) |
There was a problem hiding this comment.
Using plan.ZoneIds.String() directly may not provide the correct format for the API. This will likely result in a string representation like '["id1", "id2"]' instead of a comma-separated list. Consider extracting the actual string values and joining them with commas.
| p.SetZoneid(plan.ZoneIds.String()) | |
| zoneIds := plan.ZoneIds.Elements() | |
| zoneIdStrs := make([]string, len(zoneIds)) | |
| for i, v := range zoneIds { | |
| zoneIdStrs[i] = v.(types.String).ValueString() | |
| } | |
| p.SetZoneid(strings.Join(zoneIdStrs, ",")) |
| state.DisplayText = types.StringValue(cs.Displaytext) | ||
| } | ||
| if cs.Domainid != "" { | ||
| state.DomainIds, _ = types.SetValueFrom(ctx, types.StringType, strings.Split(cs.Domainid, ",")) |
There was a problem hiding this comment.
Error from types.SetValueFrom is ignored. This could lead to silent failures when converting domain IDs. The error should be handled properly or at least logged.
| state.NetworkRate = types.Int32Value(int32(cs.Networkrate)) | ||
| } | ||
| if cs.Zoneid != "" { | ||
| state.ZoneIds, _ = types.SetValueFrom(ctx, types.StringType, strings.Split(cs.Zoneid, ",")) |
There was a problem hiding this comment.
Error from types.SetValueFrom is ignored. This could lead to silent failures when converting zone IDs. The error should be handled properly or at least logged.
| p.SetHypervisorsnapshotreserve(int(plan.HypervisorSnapshotReserve.ValueInt32())) | ||
| } | ||
| if !plan.MaxIops.IsNull() { | ||
| p.SetMaxiops(int64(plan.MaxIops.ValueInt64())) |
There was a problem hiding this comment.
Redundant type conversion from int64 to int64. The ValueInt64() method already returns int64, so the conversion is unnecessary.
| p.SetMaxiops(int64(plan.MaxIops.ValueInt64())) | |
| p.SetMaxiops(plan.MaxIops.ValueInt64()) |
| // // | ||
| // // Licensed to the Apache Software Foundation (ASF) under one | ||
| // // or more contributor license agreements. See the NOTICE file | ||
| // // distributed with this work for additional information | ||
| // // regarding copyright ownership. The ASF licenses this file | ||
| // // to you under the Apache License, Version 2.0 (the | ||
| // // "License"); you may not use this file except in compliance | ||
| // // with the License. You may obtain a copy of the License at | ||
| // // | ||
| // // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // // | ||
| // // Unless required by applicable law or agreed to in writing, | ||
| // // software distributed under the License is distributed on an | ||
| // // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| // // KIND, either express or implied. See the License for the | ||
| // // specific language governing permissions and limitations | ||
| // // under the License. | ||
| // // |
There was a problem hiding this comment.
The license header has extra comment slashes. Lines 1-18 have double comment prefixes ('//' instead of a single '//'). This should be consistent with other files in the codebase.
| // // | |
| // // Licensed to the Apache Software Foundation (ASF) under one | |
| // // or more contributor license agreements. See the NOTICE file | |
| // // distributed with this work for additional information | |
| // // regarding copyright ownership. The ASF licenses this file | |
| // // to you under the Apache License, Version 2.0 (the | |
| // // "License"); you may not use this file except in compliance | |
| // // with the License. You may obtain a copy of the License at | |
| // // | |
| // // http://www.apache.org/licenses/LICENSE-2.0 | |
| // // | |
| // // Unless required by applicable law or agreed to in writing, | |
| // // software distributed under the License is distributed on an | |
| // // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | |
| // // KIND, either express or implied. See the License for the | |
| // // specific language governing permissions and limitations | |
| // // under the License. | |
| // // | |
| // | |
| // Licensed to the Apache Software Foundation (ASF) under one | |
| // or more contributor license agreements. See the NOTICE file | |
| // distributed with this work for additional information | |
| // regarding copyright ownership. The ASF licenses this file | |
| // to you under the Apache License, Version 2.0 (the | |
| // "License"); you may not use this file except in compliance | |
| // with the License. You may obtain a copy of the License at | |
| // | |
| // http://www.apache.org/licenses/LICENSE-2.0 | |
| // | |
| // Unless required by applicable law or agreed to in writing, | |
| // software distributed under the License is distributed on an | |
| // "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | |
| // KIND, either express or implied. See the License for the | |
| // specific language governing permissions and limitations | |
| // under the License. | |
| // |
|
|
||
| disk_offering = { | ||
| storage_type = "local" | ||
| sdfjklsdf = "sdfjks" |
There was a problem hiding this comment.
This appears to be test data with a nonsensical attribute name 'sdfjklsdf'. This will likely cause the test to fail as this is not a valid schema attribute. This should be removed or replaced with a valid attribute.
| sdfjklsdf = "sdfjks" |
| if !plan.CpuSpeed.IsNull() { | ||
| params.SetCpuspeed(int(plan.CpuSpeed.ValueInt32())) | ||
| } |
There was a problem hiding this comment.
Missing validation that CpuSpeed is required. The schema shows this field as required, but there's a null check here. Either remove the null check or make the schema field optional.
| if !plan.CpuSpeed.IsNull() { | |
| params.SetCpuspeed(int(plan.CpuSpeed.ValueInt32())) | |
| } | |
| params.SetCpuspeed(int(plan.CpuSpeed.ValueInt32())) |
|
@poddm Can you also update the documentation for this? |
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
LGTM, tested manually by creating , updating and destroy
resource "cloudstack_service_offering_constrained" "constrained1" {
display_text = "constrained1"
name = "constrained1"
// compute
cpu_speed = 2500
max_cpu_number = 10
min_cpu_number = 2
// memory
max_memory = 4096
min_memory = 1024
// other
host_tags = "test0101,test0202"
network_rate = 1024
deployment_planner = "UserDispersingPlanner"
// Feature flags
dynamic_scaling_enabled = false
is_volatile = false
limit_cpu_use = false
offer_ha = false
zone_ids = []
}
resource "cloudstack_service_offering_fixed" "fixed1" {
display_text = "fixed1"
name = "fixed1"
// compute
cpu_number = 2
cpu_speed = 2500
memory = 2048
// other
host_tags = "test0101, test0202"
network_rate = 1024
deployment_planner = "UserDispersingPlanner"
dynamic_scaling_enabled = false
is_volatile = false
limit_cpu_use = false
offer_ha = false
}
resource "cloudstack_service_offering_unconstrained" "unconstrained1" {
display_text = "unconstrained1"
name = "unconstrained1"
host_tags = "test0101,test0202"
network_rate = 1024
deployment_planner = "UserDispersingPlanner"
dynamic_scaling_enabled = false
is_volatile = false
limit_cpu_use = false
offer_ha = false
}
terraform apply
Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
+ create
Terraform will perform the following actions:
# cloudstack_service_offering_constrained.constrained1 will be created
+ resource "cloudstack_service_offering_constrained" "constrained1" {
+ cpu_speed = 2500
+ deployment_planner = "UserDispersingPlanner"
+ display_text = "constrained1"
+ dynamic_scaling_enabled = false
+ host_tags = "test0101,test0202"
+ id = (known after apply)
+ is_volatile = false
+ limit_cpu_use = false
+ max_cpu_number = 10
+ max_memory = 4096
+ min_cpu_number = 2
+ min_memory = 1024
+ name = "constrained1"
+ network_rate = 1024
+ offer_ha = false
+ zone_ids = []
}
# cloudstack_service_offering_fixed.fixed1 will be created
+ resource "cloudstack_service_offering_fixed" "fixed1" {
+ cpu_number = 2
+ cpu_speed = 2500
+ deployment_planner = "UserDispersingPlanner"
+ display_text = "fixed1"
+ dynamic_scaling_enabled = false
+ host_tags = "test0101, test0202"
+ id = (known after apply)
+ is_volatile = false
+ limit_cpu_use = false
+ memory = 2048
+ name = "fixed1"
+ network_rate = 1024
+ offer_ha = false
}
# cloudstack_service_offering_unconstrained.unconstrained1 will be created
+ resource "cloudstack_service_offering_unconstrained" "unconstrained1" {
+ deployment_planner = "UserDispersingPlanner"
+ display_text = "unconstrained1"
+ dynamic_scaling_enabled = false
+ host_tags = "test0101,test0202"
+ id = (known after apply)
+ is_volatile = false
+ limit_cpu_use = false
+ name = "unconstrained1"
+ network_rate = 1024
+ offer_ha = false
}
Plan: 3 to add, 0 to change, 0 to destroy.
Do you want to perform these actions?
Terraform will perform the actions described above.
Only 'yes' will be accepted to approve.
Enter a value: yes
cloudstack_service_offering_unconstrained.unconstrained1: Creating...
cloudstack_service_offering_fixed.fixed1: Creating...
cloudstack_service_offering_constrained.constrained1: Creating...
cloudstack_service_offering_unconstrained.unconstrained1: Creation complete after 0s [id=e88e011a-7b3e-4f8d-b7dd-d309497fa5f3]
cloudstack_service_offering_fixed.fixed1: Creation complete after 0s [id=a0789747-d1ca-4ecb-b99e-110b2c42b126]
cloudstack_service_offering_constrained.constrained1: Creation complete after 0s [id=38323d62-adcc-4858-af26-debdddb067fe]
Apply complete! Resources: 3 added, 0 changed, 0 destroyed.
|
@vishesh92, docs are updated. |
Enhanced the following resources to support the full api spec.
I split the service offering API up into multiple resources to make it a bit clearer of the different qos types